[PATCH] D96708: [llvm] Bug fix: tightlyNested() of Loop interchange

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:590
+
+  // Check loop preheader.
+  if (InnerLoopPreHeader != OuterLoopHeader) {
----------------
Is there any underlying reason that we selectively allow these instructions in `InnerLoopPreheader`, for example why `zext`? I'm wondering if there are specific reasons or is it an ad-hoc solution for now? The test program you provided does not seem to have these instructions in particular.

I have the same question for `InnerLoopExit`.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:650
 
+  if (containsUnsafeInstructionsInInnerLoop())
+    return false;
----------------
Maybe add some comment to self-explain this piece of code? If `containsUnsafeInstructionsInInnerLoop()` then it means the loops are not tightly nested? It all depends on how we define "tightly nested" in loop interchange. For example, if we have an add instruction in the inner loop preheader then `containsUnsafeInstructionsInInnerLoop()` would return true -> but having a `add %x + 1` instruction in the inner loop preheader does not necessarily mean the loops are not tightly nested? Am I missing something?


================
Comment at: llvm/test/Transforms/LoopInterchange/pr43797-lcssa-for-multiple-outer-loop-blocks.ll:9
 
+; REMARK: NotTightlyNested
+
----------------
I understand with this patch, these tests would fail legality check. I'm wondering if these tests are really not interchangable? If we did do loop interchange on the tests it would give us something wrong?


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

https://reviews.llvm.org/D96708