[PATCH] D12499: Replace ScalarEvolution based domain generation

[Michael Kruse via llvm-commits]

Meinersbur added inline comments.

================
Comment at: lib/Analysis/ScopInfo.cpp:805
@@ +804,3 @@
+
+  // Remove the arifical upper bound parameters again.
+  BoundedParts = isl_set_remove_dims(BoundedParts, isl_dim_param, 0, Dim);
----------------
artificial

================
Comment at: lib/Analysis/ScopInfo.cpp:1451
@@ -1489,6 +1450,3 @@
   Region *NonAffineSubRegion = RN->getNodeAs<Region>();
-  Loop *L = LI.getLoopFor(NonAffineSubRegion->getEntry());
-  while (L && NonAffineSubRegion->contains(L))
-    L = L->getParentLoop();
-  return L;
+  return getRegionLoop(NonAffineSubRegion, LI);
 }
----------------
Function extraction into separate commit?

================
Comment at: lib/Analysis/ScopInfo.cpp:1546
@@ -1545,3 +1513,2 @@
     isl_set *Domain = DomainMap[BB];
-    DEBUG(dbgs() << "\tVisit: " << BB->getName() << " : " << Domain << "\n");
     assert(Domain && "Due to reverse post order traversal of the region all "
----------------
Is dropping DEBUG()s this part of the patch?

================
Comment at: lib/Analysis/ScopInfo.cpp:1590
@@ +1589,3 @@
+/// optained by @p RI.
+///
+static __isl_give isl_set *
----------------
Empty comment line/missing annotations

================
Comment at: lib/Analysis/ScopInfo.cpp:1690
@@ +1689,3 @@
+/// @brief Create a map from SetSpace -> SetSpace where the dimensions @p Dim
+///        is incremented by one and all other dimensions are equal, hence:
+///             [i0, i1, i2, i3] -> [i0, i1, i2 + 1, i3]
----------------
hence -> e.g.

================
Comment at: lib/Analysis/ScopInfo.cpp:1775
@@ +1774,3 @@
+
+    auto *&HeaderBBDom = DomainMap[HeaderBB];
+    isl_set *FirstIteration =
----------------
isl_set *&
LLVM coding standards: "Use auto if and only if it makes the code more readable or easier to maintain"
In any case, if you keep auto, you can also let it derive that it's a pointer.

================
Comment at: lib/Support/SCEVValidator.cpp:356
@@ +355,3 @@
+    //              fixed we need to enable this handling again.
+    return ValidatorResult(SCEVType::INVALID);
+    /*
----------------
Adding a boolean parameter to SCEVValidator's constructor to discriminate the two cases?

I don't get why it's not compatible for domains.

================
Comment at: lib/Support/SCEVValidator.cpp:357
@@ +356,3 @@
+    return ValidatorResult(SCEVType::INVALID);
+    /*
+        assert(SRem->getOpcode() == Instruction::SRem &&
----------------
Don't commit out-commented source
Also, use 
    #if 0
    #endif
for such cases

================
Comment at: test/Isl/CodeGen/OpenMP/loop-body-references-outer-values-3.ll:42
@@ -41,3 +43,3 @@
 ; AST: #pragma omp parallel for
-; AST: for (int c0 = 0; c0 < cols; c0 += 1)
 ; AST:   Stmt_for_body(c0);
----------------
nice


http://reviews.llvm.org/D12499