[PATCH] D97828: [LoopInterchange] Disallow interchange when memory accesses are guarded by control flow (PR48057)

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:989
+
+  while (!Worklist.empty()) {
+    BasicBlock *BB = Worklist.pop_back_val();
----------------
Whitney wrote:
> ```
>   br cond, BB1, BB2
> BB1:
>   br BB3
> BB2
>   br BB3
> ```
> 
> BB3 should not guarded by a loop variant block, but this function would return BB1, BB2, and BB3.
I agree with this concern. I'm wondering if we can do this: instead of the entire worklist algorithm in this function, can we just loop over all BBs in the inner loop that are successors of loop-variant cmps, and check if they dominate the loop latch? 

Currently in loop interchange the inner loop latch is also the (only) inner loop exiting block, therefore a BB that does not dominate the inner latch can be considered as involved in control-flow divergence. This way the treatment of the above case would be seemingly easier.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1035
+      if (PHINode *PHI = dyn_cast<PHINode>(&I)) {
+        for (BasicBlock *OpBlock : PHI->blocks())
+          Guarded |= !DT->dominates(OpBlock, LoopLatch);
----------------
is `OpBlock` is not from `LoopVariantGuardedBlocks`, would interchange be legal?


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1048
+  // instructions in the worklist.
+  while (!Worklist.empty()) {
+    Instruction *Inst = Worklist.pop_back_val();
----------------
would it simplify the logic if we use only `GuardedInst` instead of both `GuardedInst` and `Worklist`, and directly check for every instruction `I` in `GuardedInst`, if any of `I` or users of `I` accesses memory then we bail out (detailed checks in the suggestion below)? 

This way we can also avoid looping over all instructions in the loop as in line 1061.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1066
+      if (GuardedInst.find(&I) != GuardedInst.end())
+        return true;
+    }
----------------
IMHO maybe we make the check more fine-grained.

1. If the value being stored is a loop-invariant, then it would not break legality.

2. Otherwise if we store (loop-variants) to the same location, then bail out. 

3. Otherwise if we store (loop-variants) not to the same location and the memory locations do not alias, then it would not break legality. I'm thinking of cases like `cond && B[i][j] = A[i][j]` inside the inner loop.

Suggestion No.3 might be less straightforward to implement, but No.1/No.2 is hopefully more straightforward to implement.


================
Comment at: llvm/test/Transforms/LoopInterchange/control-flow.ll:153
+  %0 = load i8, i8* %cond, align 1
+  %tobool = trunc i8 %0 to i1
+  br i1 %tobool, label %if.then, label %if.else
----------------
is `%tobool` a loop-invariant? I guess interchange in this case is actually legal should `%0` is a loop-invariant?

If in this IR we hoist `%0` outside of the loop nest (can we add the corresponding test case), I guess loop interchange should not bail out but with this patch it would bail out. Maybe it makes more sense to use `SCEV::isLoopInvariant()` rather than `Loop::isLoopInvariant()` in function `getLoopVariantGuardedBlocks()`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97828/new/

https://reviews.llvm.org/D97828