drop checkValidIslSchedule

This function dates back from a time in prehistory when
internal isl data structures (schedule trees) were
being created from the outside, subverting the internal checks
in isl.  Part of the removed comments still mentions
the possibility of constructing an invalid isl schedule.
Since then, the isl schedule tree construction
has been changed to use the proper interface.
There is therefore no longer any need to check that the constructed
isl schedule tree would be valid since isl should perform
the appropriate checks.  If any of those checks are missing
from isl then they should be added there.

The removed function is too low-level, incomplete (FIXMEs) and
has a high risk of getting out of sync with isl.
More generally, it does not make sense for TC to guess
whether the result of a conversion to isl schedule trees
would be valid if it does not control the conversion.
It may make sense to add a function that checks
whether it is possible to convert a TC schedule tree
to an isl schedule tree instead.

Author: Sven Verdoolaege

tc/core/polyhedral/codegen_llvm.cc

@@ -669,7 +669,6 @@ IslCodegenRes codegenISL(const Scop& scop) {
     return collectIteratorMaps(n, b, uv, scop, stmtSubscripts);
   };
 
-  checkValidIslSchedule(scop.scheduleRoot());
   auto schedule = detail::toIslSchedule(scop.scheduleRoot());
   auto astBuild = isl::ast_build(schedule.get_ctx());
   astBuild = astBuild.set_at_each_domain(collect);

tc/core/polyhedral/cuda/codegen.cc

@@ -738,7 +738,6 @@ string emitCudaKernel(
     return collectIteratorMaps(n, b, &nodeInfoMap);
   };
 
-  checkValidIslSchedule(mscop.schedule());
   auto schedule = detail::toIslSchedule(mscop.schedule());
   auto astBuild = isl::ast_build(schedule.get_ctx());
   astBuild = astBuild.set_at_each_domain(collect);

tc/core/polyhedral/schedule_isl_conversion.cc

@@ -243,7 +243,6 @@ isl::schedule_node extendChild(
  * then recursively add nodes corresponding to the descendants of "root".
  */
 isl::schedule toIslSchedule(const ScheduleTree* root) {
-  checkValidIslSchedule(root);
   auto domain = root->elemAs<ScheduleTreeElemDomain>();
   CHECK(domain) << "Root node should be domain node" << *root;
   auto node = isl::schedule_node::from_domain(domain->domain_);
@@ -289,235 +288,6 @@ bool validateSchedule(isl::schedule sc) {
   return validateSchedule(fromIslSchedule(sc).get());
 }
 
-namespace {
-
-// Get the parametric space in which the relation contained by the given tree
-// are defined.
-isl::space definitionParamSpace(const ScheduleTree* node) {
-  auto space = isl::space(node->ctx_, 0);
-  switch (node->type_) {
-    // mapping_filter is a filter for ISL
-    // TODO: this switch is too ISL-ish and is not meant for subtyping
-    // extensions. Replace by if (node->elemAsBase<T>(...))
-    case detail::ScheduleTreeType::MappingFilter:
-    case detail::ScheduleTreeType::Filter: {
-      auto filterNode = node->elemAsBase<ScheduleTreeElemFilter>();
-      space = filterNode->filter_.get_space().params();
-      break;
-    }
-    case detail::ScheduleTreeType::Band: {
-      auto bandNode = node->elemAs<ScheduleTreeElemBand>();
-      space = bandNode->mupa_.get_space().params();
-      break;
-    }
-    case detail::ScheduleTreeType::Extension: {
-      auto extensionNode = node->elemAs<ScheduleTreeElemExtension>();
-      space = extensionNode->extension_.get_space().params();
-      break;
-    }
-    case detail::ScheduleTreeType::Domain: {
-      auto domainElem = node->elemAs<ScheduleTreeElemDomain>();
-      space = domainElem->domain_.get_space().params();
-      break;
-    }
-    case detail::ScheduleTreeType::Context: {
-      auto contextElem = node->elemAs<ScheduleTreeElemContext>();
-      space = contextElem->context_.get_space().params();
-      break;
-    }
-
-    // Other types of nodes do not have any potentially parametric expression.
-    case detail::ScheduleTreeType::Any:
-    case detail::ScheduleTreeType::None:
-    case detail::ScheduleTreeType::Set:
-    case detail::ScheduleTreeType::Sequence:
-    case detail::ScheduleTreeType::ThreadSpecificMarker:
-      break;
-  }
-  return space;
-}
-
-bool refersToUndefinedParameters(
-    const ScheduleTree* relativeRoot,
-    const ScheduleTree* node) {
-  using namespace polyhedral::detail;
-
-  // Assuming no parameters are introduced above root.
-  if (node == relativeRoot) {
-    return false;
-  }
-
-  // Domain and context can introduce new parameters.
-  if (node->elemAs<ScheduleTreeElemDomain>() ||
-      node->elemAs<ScheduleTreeElemContext>()) {
-    return false;
-  }
-
-  // Collect all ancestors that are allowed to introduce new parameters, i.e.
-  // domain and context nodes.  Collect the parameters they introduce in a
-  // space.
-  auto paramSpace = isl::null<isl::space>();
-  for (auto anc : node->ancestors(relativeRoot)) {
-    auto contextNode = anc->elemAs<ScheduleTreeElemContext>();
-    auto domainNode = anc->elemAs<ScheduleTreeElemDomain>();
-    if (!contextNode && !domainNode) {
-      continue;
-    }
-
-    auto space = contextNode ? contextNode->context_.get_space()
-                             : domainNode->domain_.get_space();
-    space = space.params();
-    paramSpace = paramSpace.get() ? paramSpace.align_params(space) : space;
-  }
-
-  CHECK(paramSpace.get()) << "no parent context or domain node found";
-  if (!paramSpace.get()) {
-    return true;
-  }
-
-  // The space in which tree's relations are defined should not involve
-  // parameters that were not present in its domain or context ancestors. If
-  // the tree has no relations, it cannot refer to undefined parameters.
-  auto space = definitionParamSpace(node);
-  if (space.dim(isl::dim_type::param) == 0) {
-    return false;
-  }
-
-  // If space uses some parameters that are not available in paramSpace,
-  // they will be introduced into paramSpace, making its dimension larger.
-  auto definedParams = paramSpace.dim(isl::dim_type::param);
-  paramSpace = paramSpace.align_params(space);
-  return paramSpace.dim(isl::dim_type::param) > definedParams;
-}
-
-// Note that this uses union_set as "optional" where nullptr value means
-// there's no actual value, rather than an error.
-isl::union_set nodeDomain(const ScheduleTree* node) {
-  if (auto domainElem = node->elemAs<ScheduleTreeElemDomain>()) {
-    return domainElem->domain_;
-  } else if (auto bandElem = node->elemAs<ScheduleTreeElemBand>()) {
-    return bandElem->mupa_.domain();
-  } else if (auto filterElem = node->elemAsBase<ScheduleTreeElemFilter>()) {
-    return filterElem->filter_;
-  } else if (auto extensionElem = node->elemAs<ScheduleTreeElemExtension>()) {
-    // FIXME: these are the points that are _introduced_...  they are inactive
-    // until now...
-    // does the extension have a domain?
-    return extensionElem->extension_
-        .range(); // FIXME: do we need to restrict its domain first?
-  }
-  return isl::null<isl::union_set>();
-}
-} // namespace
-
-void checkValidIslSchedule(const ScheduleTree* root_) {
-  using namespace polyhedral::detail;
-
-  // 1. The root node is always of type domain or extension.
-  auto domainRoot = root_->elemAs<ScheduleTreeElemDomain>();
-  auto extensionRoot = root_->elemAs<ScheduleTreeElemDomain>();
-  CHECK(domainRoot || extensionRoot)
-      << "root must be a domain or an extension" << *root_;
-
-  for (auto node : ScheduleTree::collect(root_)) {
-    auto activeInstances = activeDomainPoints(root_, node);
-    auto nodeDomainPoints = nodeDomain(node);
-
-    // 2. Only set or sequence nodes are allowed to have multiple children and
-    // these children must be filter nodes.
-    auto nodeIsSet = node->elemAs<ScheduleTreeElemSet>() != nullptr;
-    auto nodeIsSequence = node->elemAs<ScheduleTreeElemSequence>() != nullptr;
-    auto nChildren = node->numChildren();
-
-    // 4. Nodes should not refer to inactive domain points.
-    if (nodeDomainPoints.get()) {
-      if (!nodeDomainPoints.is_subset(activeInstances)) {
-        LOG_IF(WARNING, tc::FLAGS_schedule_tree_verbose_validation)
-            << "node refers to inactive domain points: active "
-            << activeInstances << " found: " << nodeDomainPoints << " in\n"
-            << *node;
-      }
-    }
-
-    if (!nodeIsSet && !nodeIsSequence) {
-      CHECK_LE(nChildren, 1u)
-          << "only sequence or set nodes can have multiple children" << *node;
-    } else {
-      auto filters = isl::null<isl::union_set>();
-      for (auto child : node->children()) {
-        auto filterElem = child->elemAsBase<ScheduleTreeElemFilter>();
-        auto childIsFilter = filterElem != nullptr;
-        CHECK(childIsFilter)
-            << "only filter nodes allowed as children of sequence or set node"
-            << *child;
-
-        filters = filters.get() ? filters.unite(filterElem->filter_)
-                                : filterElem->filter_;
-      }
-      CHECK(filters.get()) << "set/sequence node must have at least one child"
-                           << *node;
-
-      // 5. The union of filters must cover all active domain points.
-      CHECK(activeInstances.is_subset(filters))
-          << "filters must cover all active domain points; active "
-          << activeInstances << " filtered: " << filters << " in\n"
-          << *node;
-    }
-
-    // 3. Nodes should not refer to parameters that are not declared in context
-    // nodes above.
-    bool usesUndefinedParams = refersToUndefinedParameters(root_, node);
-    CHECK(!usesUndefinedParams)
-        << "non-context node introduces new parameters" << *node;
-
-    // 7. Band schedules should be total on all active domain points.
-    // Only check if an actual domain is specified.
-    // In particular, 0D bands do not necessarily specify a domain.
-    if (auto bandElem = node->elemAs<ScheduleTreeElemBand>()) {
-      auto scheduleDefinitionDomain = bandElem->mupa_.domain();
-      if (!scheduleDefinitionDomain.is_params()) {
-        CHECK(activeInstances.is_subset(scheduleDefinitionDomain))
-            << "schedule should be total on the active domain points: active"
-            << activeInstances
-            << " schedule defined over: " << scheduleDefinitionDomain << " in\n"
-            << *node;
-      }
-    }
-
-    // 8. Extension nodes should not introduce any elements that are already
-    // active domain elements.
-    // 10. Anchored nodes match the flattened space of the outer bands.
-    if (auto extensionElem = node->elemAs<ScheduleTreeElemExtension>()) {
-      auto introducedInstances =
-          extensionElem->extension_
-              .range(); // FIXME: restrict domain to the partial schedule??
-      CHECK(introducedInstances.intersect(activeInstances).is_empty())
-          << "extension node should not introduce elements that are already "
-             "active domain elements: active "
-          << activeInstances << " introduced: " << introducedInstances
-          << " in\n"
-          << *node;
-      // FIXME: we probably want to check dim_ids for an exact match
-      auto depth = node->scheduleDepth(root_);
-      auto extension = extensionElem->extension_;
-      for (auto const& e : isl::UnionAsVector<isl::union_map>(extension)) {
-        CHECK_EQ(depth, e.dim(isl::dim_type::in))
-            << "the input dimensionality of the extension map should "
-               "correspond to the schedule depth"
-            << *node;
-      }
-    }
-
-    // 10. Anchored nodes match the flattened space of the outer bands.
-    if (auto contextElem = node->elemAs<ScheduleTreeElemContext>()) {
-      auto depth = node->scheduleDepth(root_);
-      CHECK_EQ(depth, contextElem->context_.dim(isl::dim_type::set))
-          << "the dimensionality of the context should correspond "
-             "to the schedule depth"
-          << *node;
-    }
-  }
-}
 } // namespace detail
 } // namespace polyhedral
 } // namespace tc

tc/core/polyhedral/schedule_isl_conversion.h

@@ -33,7 +33,6 @@ isl::schedule toIslSchedule(const ScheduleTree* root_);
 std::unique_ptr<ScheduleTree> fromIslSchedule(isl::schedule schedule);
 bool validateSchedule(const ScheduleTree* st);
 bool validateSchedule(isl::schedule schedule);
-void checkValidIslSchedule(const detail::ScheduleTree* root_);
 } // namespace detail
 } // namespace polyhedral
 } // namespace tc

tc/core/polyhedral/schedule_tree.h

@@ -104,36 +104,6 @@ namespace detail {
 //
 // ScheduleTree does NOT impose any structure requirements on the tree, e.g.
 // those of ISL.  A tree with a null child is ill-formed.
-//
-// Note that we do not enforce the isl schedule tree invariants [1] at the API
-// level.  Instead, we provide a function checkValidIslSchedule() to verify
-// whether a schedule that has a given ScheduleTree as root can be converted to
-// an isl::schedule.  Note that, for compatibility reasons, the current
-// implementation may also create isl::schedules that do not maintain isl
-// schedule tree guarantees.  The behavior of isl calls on such schedules is
-// undefined.
-//
-// The following isl invariants can be checked
-// 1. root is domain/extension
-// 2. only sequence/set have multiple children, these children are filters
-// 3. nodes do not refer to parameters that were not previously introduced by a
-//    context or a domain node
-// 4. nodes do not refer to inactive domain points, i.e. those that were
-//    filtered away (warning only)
-// 5. union of filters contains all active domain elements
-// 6. domain of an expansion contains all active domain elements
-// 7. partial schedule of a band node is total for all active domain elements
-// 8. extension nodes do not introduce any elements that are already active
-//    domain elements
-// 9. (not enforced)
-// 10. that anchored nodes match the flattened space of the outer bands
-//
-// TODO(ftynse): implement bool checkValidIslSchedule() to check schedule
-// structure without failing the run.
-//
-//
-// [1] Verdoolaege, Guelton, Grosser & Cohen (2014). "Schedule trees". In
-// IMPACT 2014.
 //////////////////////////////////////////////////////////////////////////////
 
 std::ostream& operator<<(std::ostream& os, const ScheduleTree& tree);