[PATCH] D102300: [LoopInterchange] Check lcssa phis in the inner latch in scenarios of multi-level nested loops

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1050
+    for (auto *U : PHI.users()) {
+      Instruction *UI = dyn_cast<Instruction>(U);
+      if (InnerLoopLatch == UI->getParent())
----------------
Whitney wrote:
> Can user of PHI be not an instruction? If not, change dyn_cast to cast.
Thanks, will update to `cast`.


================
Comment at: llvm/test/Transforms/LoopInterchange/innermost-latch-uses-values-in-middle-header.ll:16
+; CHECK: Not interchanging loops. Cannot prove legality.
+define void @innermost_latch_uses_values_in_middle_header() {
+entry:
----------------
Whitney wrote:
> Is it possible to show the problem with input IR that the innermost and the middle loop is already interchanged?
Thanks for the comment! The difficulty is that loop interchange works on loops ordered from the innermost loop toward the outermost loop. Whenever we fail the legality check for a pair of loops, we just bail out the entire pass not trying to interchange the rest of loop nest. The pass does not "skip over" the first pair of candidate loops and go directly to the next one.

With the input IR that the innermost and the middle loop is already interchanged, when the pass starts working on this loop nest, it would do one of the following:

1) The pass either determines it does not want to interchange the (new) innermost and the (new) middle loop likely due to profitability issue and will just bail out, not interchanging anything.

2) (Even if we manually change the loop access pattern in the IR to make it pass profitability), it would interchange the the (new) innermost and the (new) middle loop which would result in a new IR that does not expose the problem we would like to expose.

Therefore I thought the current test case might be a in a sense a straightforward one to catch the problem.  
- With the trunk code, it run the 1st interchange and crashes on the 2nd interchange. 
- With this patch, it run the 1st interchange and bails out the 2nd interchange.

I'm wondering if what I described above makes sense to you? Please correct me if I missed something.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102300