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

[Johannes Doerfert via llvm-commits]

jdoerfert marked 9 inline comments as done.
jdoerfert added a comment.

Comments and the new version will follow shortly.


================
Comment at: include/polly/Support/ScopHelper.h:82
@@ +81,3 @@
+
+/// @brief Wrapper arround the SCEVExpander extended to all SCEV additions.
+///
----------------
grosser wrote:
> "all SCEV additions"?
> 
> you mean extended "to handle Polly specific operations not handled by SCEVExpander"?
Yes, I rephrased.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:127
@@ -130,1 +126,3 @@
+        Value *Expanded =
+            expandCodeFor(S, SE, DL, "polly", NewScev, Old->getType(), IP);
 
----------------
grosser wrote:
> 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?
No and I do not think we would crash here anyway. I think only the SDiv/SRem is a parameter of the SCoP case can crash. However, I changed all Expander locations for two reasons:
  1) We have a clear interface now that hides how we actually expand code.
  2) We have one location where we need to handle new features later on.

If you think it is better to only use the new interface where it is necessary I am almost certain this change and the ones in the IslExprBuilder can be avoided.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:387
@@ -386,3 +386,3 @@
 
-    if (R.contains(UI))
+    if (R.contains(UI) || EnteringBB == UI->getParent())
       continue;
----------------
grosser wrote:
> 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).
I am unsure because I cannot reproduce it atm. I will remove it run lnt and if it passes I will just keep this in mind. 

================
Comment at: lib/Support/ScopHelper.cpp:270
@@ +269,3 @@
+    if (!S.getRegion().contains(I))
+      E = visit(E);
+    return Expander.expandCodeFor(E, Ty, I);
----------------
grosser wrote:
> 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?
Like I said above. I don't think so. Even if that might be the case, the current handling might not work then. For now I see 3 cases we expands SCEVs:
  1) IslExprBuilder: Here we create (mainly) loop bounds etc. (inside the region) and the problem should not occure since we generate the full expression (except the parameters) from scratch.
  2) BlockGenerator: Here we synthezise values during code generation (inside the region) but again, everything except the parameters are generated from scratch (afaik).
  3) IslNodeBuilder: Here we generate code for the parameters (in front of the region) and we can crash. However, only if the parameters contains instructions in the region.

If the above observation is correct the code should be too. If not or we extend/change Polly at some point the new structure should allows us to modify the behavior of the SCEV expansion pretty easily.

================
Comment at: lib/Support/ScopHelper.cpp:294
@@ +293,3 @@
+    if (!StartIP)
+      setStartIP();
+
----------------
Meinersbur wrote:
> Introduce getStartIP() which sets StartIP if nullptr ?
Done in the constructor.


http://reviews.llvm.org/D12066