Merge pull request #427 from facebookresearch/pr/valid_schedule

drop checkValidIslSchedule

Author: ftynse

tc/core/polyhedral/codegen_llvm.cc

@@ -667,7 +667,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

@@ -785,7 +785,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);