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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour 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;
----------------
`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?


================
Comment at: llvm/test/Transforms/LoopInterchange/inner-indvar-depend-on-outer-indvar.ll:4
+
+ at A = common global [100 x [100 x i64]] zeroinitializer
+
----------------
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;
```



================
Comment at: llvm/test/Transforms/LoopInterchange/inner-indvar-depend-on-outer-indvar.ll:20
+for1.header:
+  %j23 = phi i64 [ 0, %entry ], [ %j.next24, %for1.inc10 ]
+  br label %for2
----------------
name this "i" to be consistent with the example above.


================
Comment at: llvm/test/Transforms/LoopInterchange/inner-indvar-depend-on-outer-indvar.ll:57
+for1.header:
+  %j23 = phi i64 [ 0, %entry ], [ %j.next24, %for1.inc10 ]
+  br label %for2
----------------
name this "i"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101305