[PATCH] D12066: Introduce the ScopExpander as a SCEVExpander replacement

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Johannes,

this patch goes in the right direction. I have a couple of comments, but most of them are minor. Can you submit another version, such that I can have a final look. (mostly interested in the handleOutsideUsers test case).

Thank you,
Tobias


================
Comment at: include/polly/CodeGen/IslExprBuilder.h:96
@@ -94,2 +95,3 @@
   /// @param Expander  A SCEVExpander to create the indices for multi
   ///                  dimensional accesses.
+  IslExprBuilder(Scop &S, PollyIRBuilder &Builder, IDToValueTy &IDToValue,
----------------
Expander is not a parameter any more.

================
Comment at: include/polly/Support/ScopHelper.h:82
@@ +81,3 @@
+
+/// @brief Wrapper arround the SCEVExpander extended to all SCEV additions.
+///
----------------
"all SCEV additions"?

you mean extended "to handle Polly specific operations not handled by SCEVExpander"?

================
Comment at: include/polly/Support/ScopHelper.h:84
@@ +83,3 @@
+///
+/// This wrapper will internaly call the SCEVExpander but also make sure that
+/// all additional features not represented in SCEV (e.g., SDiv/SRem are not
----------------
internally

================
Comment at: include/polly/Support/ScopHelper.h:89
@@ +88,3 @@
+/// The parameters are the same as for the creation of a SCEVExpander as well
+/// as the call to SCEVExpander::expandCodeFor.
+llvm::Value *expandCodeFor(Scop &S, llvm::ScalarEvolution &SE,
----------------
It might still make sense to document the parameters briefly. At least I do not know them by heart.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:127
@@ -130,1 +126,3 @@
+        Value *Expanded =
+            expandCodeFor(S, SE, DL, "polly", NewScev, Old->getType(), IP);
 
----------------
Do we happen to have a test case covering this use? Meaning a loop-variant srem node with data-dependences accross basic-blocks that would crash without this addition?

================
Comment at: lib/CodeGen/BlockGenerators.cpp:387
@@ -386,3 +386,3 @@
 
-    if (R.contains(UI))
+    if (R.contains(UI) || EnteringBB == UI->getParent())
       continue;
----------------
If I drop this change, all test cases pass. Could you possibly add a test case that illustrates why this change is needed. (It is not 100% clear to me).

================
Comment at: lib/Support/ScopHelper.cpp:258
@@ +257,3 @@
+
+struct ScopExpander : SCEVVisitor<ScopExpander, const SCEV *> {
+  friend struct SCEVVisitor<ScopExpander, const SCEV *>;
----------------
Maybe add a comment that explains why we have to extend the SCEVExpander (we can probably resue the replies to Hal and Sanjay).

================
Comment at: lib/Support/ScopHelper.cpp:268
@@ +267,3 @@
+    // SCEVExpander, otherwise we will stop at all unknowns in the SCEV and if
+    // needed replace them by copies computed in the entering block.
+    if (!S.getRegion().contains(I))
----------------
Why do we not use the ScopExpander inside the region. Even in the region we may generate code at a location which is not dominated by all SCEVUnknown expressions in the SCEV.

================
Comment at: lib/Support/ScopHelper.cpp:270
@@ +269,3 @@
+    if (!S.getRegion().contains(I))
+      E = visit(E);
+    return Expander.expandCodeFor(E, Ty, I);
----------------
Why don't we use the ScopExpander in the region? Even inside the region we
may work at places where I does not dominate all SCEVUnknowns, no?


http://reviews.llvm.org/D12066