[PATCH] D101305: [LoopInterchange] Fix legality for triangular loops

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:714
+    const SCEV *S = SE->getSCEV(Right);
+    if (!SE->isLoopInvariant(S, OuterLoop))
+      return false;
----------------
bmahjour wrote:
> `isLoopInvariant` considers any instruction that is not known to SCEV as invariant only if that instruction is not in the loop. I believe most of the interchange test cases use constant upper bounds....did you try some with non-constant upper bounds (eg. loading a global, where the global is not LICMed out of the outer loop) to make sure we can still interchange them?
Thanks for the comment! I see your concern and I checked that if we use an upper bound that is a global loaded inside the loop body and not LICMed out of the loop, SCEV would determine this upper bound to be non-constant and hence we would not do interchange.

I'd like to mention that in this function `isLoopStructureUnderstood()`, on line 655 which is right before my added code, when we try to detect `for(int j=i; ; )`, this pass used `isLoopInvariant()` which has the same problem as you mentioned. I'm guessing maybe at least to some extend, loop interchange has the assumption that loop-invariants are already hoisted outside the loop, so we are just fine in the sense that we are aligned with the previous logic?

Still I agree that Loop Interchange is sensitive to dependencies created by LICM hoisting code out of the inner loop. This might be a 'phase ordering' problem and unfortunately there might be no satisfying solution. I'm wondering if what I described above makes sense to you? If not, I'm entirely open to any suggestions you have, i.e., other approaches to determine whether the inner loop upper bounds are out loop invariant..





================
Comment at: llvm/test/Transforms/LoopInterchange/inner-indvar-depend-on-outer-indvar.ll:4
+
+ at A = common global [100 x [100 x i64]] zeroinitializer
+
----------------
bmahjour wrote:
> do we have coverage elsewhere for cases where the operands of the compare are reversed? If not please add one here:
> 
> negative test:
> ```
> ;;  for(int i=0;i<100;i++)
> ;;    for(int j=0;i>j;j++)
> ;;      A[j][i] = A[j][i]+k;
> ```
> positive test:
> ```
> ;;  for(int i=0;i<100;i++)
> ;;    for(int j=0;N>j;j++)
> ;;      A[j][i] = A[j][i]+k;
> ```
> 
Thanks for the comment, will update the test cases soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101305