[PATCH] D39971: Port ScopInfo to the isl cpp bindings

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

LGTM, only nitpicks. Thanks for your work.



================
Comment at: include/polly/ScheduleOptimizer.h:74-75
 
-} // namespace polly
-
 class ScheduleTreeOptimizer {
----------------
Is this change related?

I think it is a good idea to include `ScheduleTreeOptimizer` in the Polly namespace, can you commit it separately?


================
Comment at: include/polly/ScopInfo.h:1696-1702
+  /// Isl context.
+  ///
+  /// We need a shared_ptr with reference counter to delete the context when all
+  /// isl objects are deleted. We will distribute the shared_ptr to all objects
+  /// that use the context to create isl objects, and increase the reference
+  /// counter. By doing this, we guarantee that the context is deleted when we
+  /// delete the last object that creates isl objects with the context.
----------------
Why the move?


[Note] Seems to be wrong in the original comment:

> By doing this, we guarantee that the context is deleted when we delete the last object that creates isl objects with the context.

Isn't the point that the isl_ctx is deleted /only after/ all its isl objects are released? I mean, the isl objects don't participate in this `shared_ptr` and therefore cannot trigger the release of the isl_ctx.


================
Comment at: include/polly/Support/GICHelper.h:180-215
+static inline std::string stringFromIslObj(isl::map map) {
+  return stringFromIslObj(map.keep());
+}
+static inline std::string stringFromIslObj(isl::union_map umap) {
+  return stringFromIslObj(umap.keep());
+}
+static inline std::string stringFromIslObj(isl::set set) {
----------------
For consistency with the other `stringFromIslObj` functions, shouldn't these be defined in the `.cpp` file?

The isl objects have `to_str()` methods, and we have overloaded the `<<` operators in `ISLOStream.h`; consider using them instead. 


================
Comment at: lib/Analysis/ScopInfo.cpp:2212
 void Scop::buildContext() {
-  isl_space *Space = isl_space_params_alloc(getIslCtx(), 0);
-  Context = isl_set_universe(isl_space_copy(Space));
-  InvalidContext = isl_set_empty(isl_space_copy(Space));
-  AssumedContext = isl_set_universe(Space);
+  auto Space = isl::space::params_alloc(getIslCtx(), 0);
+  Context = isl::set::universe(Space);
----------------
[Style] No almost-always-auto.


================
Comment at: lib/Analysis/ScopInfo.cpp:2558
   int LD = getRelativeLoopDepth(L);
-  auto *S = isl_set_universe(isl_space_set_alloc(getIslCtx(), 0, LD + 1));
+  auto *S = isl_set_universe(isl_space_set_alloc(getIslCtx().get(), 0, LD + 1));
 
----------------
Consider `isl::set::universe`


================
Comment at: lib/Analysis/ScopInfo.cpp:3397-3398
 
   // Explicitly release all Scop objects and the underlying isl objects before
   // we release the isl context.
   Stmts.clear();
----------------
This comment maybe belongs to the beginning.


================
Comment at: lib/Analysis/ScopInfo.cpp:4189-4190
+  PositiveContext = addNonEmptyDomainConstraints(PositiveContext);
+  if (!PositiveContext)
+    return false;
+
----------------
This is the changed part, right? Under which conditions does `addNonEmptyDomainConstraints` return nullptr? It's doxygen doesn't mention it.


================
Comment at: lib/Analysis/ScopInfo.cpp:4354
     // assumption.
-    isl_set *S = AS.Set;
+    isl_set *S = AS.Set.copy();
     if (AS.Sign == AS_RESTRICTION)
----------------
No `isl::set S`?

Edit: From other places I realise you did not convert everything within functions, we do some other time.


================
Comment at: lib/CodeGen/IslAst.cpp:678-679
                                ScopStandardAnalysisResults &SAR) {
-  return {S, SAM.getResult<DependenceAnalysis>(S, SAR).getDependences(
-                 Dependences::AL_Statement)};
+  return {S,
+          SAM.getResult<DependenceAnalysis>(S, SAR).getDependences(
+              Dependences::AL_Statement)};
----------------
Did something change here?


================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:1074-1075
           auto Address = MemInst ? MemInst.getPointerOperand() : nullptr;
-          if (Address && SE.getUnknown(UndefValue::get(Address->getType())) ==
-                             SE.getPointerBase(SE.getSCEV(Address))) {
+          if (Address &&
+              SE.getUnknown(UndefValue::get(Address->getType())) ==
+                  SE.getPointerBase(SE.getSCEV(Address))) {
----------------
Did something change here? clang-format going rogue?


================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:1368
     // we need to refine the ExecutionCtx.
-    isl_set *BaseExecutionCtx = isl_set_copy(BaseIAClass->ExecutionContext);
-    ExecutionCtx = isl_set_intersect(ExecutionCtx, BaseExecutionCtx);
+    auto BaseExecutionCtx = BaseIAClass->ExecutionContext;
+    ExecutionCtx = ExecutionCtx.intersect(BaseExecutionCtx);
----------------
[Style] no almost-always-auto.

(I only care about introducing new ones, it is up to you if you want to remove existing autos)


================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:1385
         // and we need to refine the ExecutionCtx.
-        isl_set *BaseExecutionCtx = isl_set_copy(BaseIAClass->ExecutionContext);
-        ExecutionCtx = isl_set_intersect(ExecutionCtx, BaseExecutionCtx);
+        auto BaseExecutionCtx = BaseIAClass->ExecutionContext;
+        ExecutionCtx = ExecutionCtx.intersect(BaseExecutionCtx);
----------------
[Style] no almost-always-auto.


================
Comment at: lib/Support/GICHelper.cpp:27
 using namespace llvm;
+using namespace polly;
 
----------------
Is this required?


================
Comment at: unittests/ScheduleOptimizer/ScheduleOptimizerTest.cpp:15
 
+using namespace polly;
 using namespace isl;
----------------
Is this required?


https://reviews.llvm.org/D39971