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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
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;
----------------
[Suggestion] Wouldn't is be easier to enumerate over `Scop::getBoxedLoops()` and check those whether they are contained in the stmt?


================
Comment at: lib/Analysis/ScopInfo.cpp:1749-1753
+  LoopInfo *LI = getParent()->getLI();
+
+  for (Loop *L : make_range(LI->begin(), LI->end()))
+    if (contains(L))
+      return true;
----------------
I assume this is for the case `L==nullptr`. Could be put into the else branch. of `if (L)`


================
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())) {
----------------
[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.


================
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";
----------------
Message should be changed to the new meaning.


================
Comment at: test/DeLICM/reject_storeinsubregion.ll:78-85
+; CHECK:      DeLICM result:
+; CHECK-NEXT: Statistics {
+; CHECK-NEXT:     Compatible overwrites: 1
+; CHECK-NEXT:     Overwrites mapped to:  0
+; CHECK-NEXT:     Value scalars mapped:  0
+; CHECK-NEXT:     PHI scalars mapped:    0
+; CHECK-NEXT: }
----------------
This was a check the case when it does _not_ work. Please ensure that we still have a test case for the not-allowed case.


================
Comment at: test/ForwardOpTree/forward_region.ll:10
+; bodyB_entry:
+;   if (undef)
+; body_true:
----------------
This condition does not correspond to the IR below.


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


https://reviews.llvm.org/D37298