[PATCH] D37298: [ForwardOpTree] Allow forwarding in the presence of region statements

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

Thank you. Looking into it.



================
Comment at: lib/Analysis/ScopInfo.cpp:1740-1756
+bool ScopStmt::hasBoxedLoops() const {
+  Loop *L = getSurroundingLoop();
+
+  if (L) {
+    for (Loop *L : L->getSubLoops())
+      if (contains(L))
+        return true;
----------------
Meinersbur wrote:
> [Suggestion] Wouldn't is be easier to enumerate over `Scop::getBoxedLoops()` and check those whether they are contained in the stmt?
This would grow linearly in the size of the scop, whereas the current approach hopefully would not.


================
Comment at: lib/Transform/ZoneAlgo.cpp:379
     // might be in a boxed loop.
-    if (Stmt->isRegionStmt() &&
+    if (Stmt->hasBoxedLoops() &&
         !isl_union_map_is_disjoint(Loads.keep(), AccRel.keep())) {
----------------
Meinersbur wrote:
> [Serious] Boxed loops are not the only problematic case. Consider within a region stmt:
> ```
>   br label %StmtB
> 
> StmtA:
>   %a= load %A
>   br label %StmtC  
> 
> StmtB:
>   store %A 
>   br label %StmtA  
> 
> StmtC:
>   ...
> ```
> Iteration order might be StmtA first, then StmtB, although StmtB is executed before StmtA. It should be rejected, but is not.
OK. What would you suggest to check instead?


================
Comment at: lib/Transform/ZoneAlgo.cpp:381-384
       DEBUG(dbgs() << "WRITE in non-affine subregion not supported\n");
       OptimizationRemarkMissed R(PassName, "StoreInSubregion",
                                  MA->getAccessInstruction());
       R << "store is in a non-affine subregion";
----------------
Meinersbur wrote:
> Message should be changed to the new meaning.
OK.


================
Comment at: test/ForwardOpTree/forward_region.ll:10
+; bodyB_entry:
+;   if (undef)
+; body_true:
----------------
Meinersbur wrote:
> This condition does not correspond to the IR below.
I just moved the old test case. Will cross-check.


================
Comment at: test/ForwardOpTree/noforward_region.ll:1
-; RUN: opt %loadPolly -polly-optree -analyze < %s | FileCheck %s -match-full-lines
-;
----------------
Meinersbur wrote:
> Why is this deleted? There are still cases in regions that are not allowed.
OK.


https://reviews.llvm.org/D37298