[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;
----------------
congzhe wrote:
> 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..
> 
> 
> 
That's true. We already have dependency on LICM today, although the existing code only checks for the initial value of the induction variable. With this patch that dependency is a bit expanded to a more common case where the loop upper bound is a variable. Having said that, since LICM already runs before interchange this is not going to be a problem in practice. The real deficiency is in SCEV's `isLoopInvariant` and how various passes rely on it. Not sure if this is feasible but theoretically one could factor out the decision logic from LICM and put it in a utility that can be called by transforms that need to check for invariance but don't want to rely on LICM. We can look into that if the need arises.


================
Comment at: llvm/test/Transforms/LoopInterchange/inner-indvar-depend-on-outer-indvar.ll:24
+  %i = phi i64 [ 0, %entry ], [ %i.next, %for1.inc10 ]
+  %upperbound = load i64, i64* @B
+  br label %for2
----------------
I don't see this being used!


================
Comment at: llvm/test/Transforms/LoopInterchange/inner-indvar-depend-on-outer-indvar.ll:108
+  %j.next = add nuw nsw i64 %j, 1
+  %exitcond = icmp ne i64 %j, %i
+  br i1 %exitcond, label %for2, label %for1.inc10
----------------
This should be changed to `%exitcond = icmp ne i64 %i, %j` to be different from `interchange_01`.


================
Comment at: llvm/test/Transforms/LoopInterchange/inner-indvar-depend-on-outer-indvar.ll:144
+  %j.next = add nuw nsw i64 %j, 1
+  %exitcond = icmp ne i64 %j, %0
+  br i1 %exitcond, label %for2, label %for1.inc10
----------------
`%exitcond = icmp ne i64 %0, %j`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101305