[PATCH] D110972: [DA] Handle mismatching loop levels by updating the numbering scheme

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

The test fails for me when applying the patch. Is D71539 <https://reviews.llvm.org/D71539> supposed to be a parent patch?

  Is the loop nest in the example supposed to look like this?

for (L1 <https://reviews.llvm.org/L1>) {

  for (L2) {
    s1 = ...;
  }
  A[s1]; // getScev(s1) = SCEVAddRecExp<L1,...>
  for (L3) {
    A[s2];
  }

}

  The problem seems to be that DA assumes that `s1` depends on the induction variable of `L1`, even though the access is located outside of it. `getSCEVAtScope` may resolve it to a computed value, but may not if it does not understand the exit condition. In `checkSubscript` this is handled by calling `isLoopInvariant` first. However, the loop-invariance is only checked if there is no SCEVAddRecExpr. Might the problem be that an SCEVAddRecExpr may actually be loop-invariant, if evaluated outside the loop's scope?



================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3590
   unsigned Pairs = 1;
-  SmallVector<Subscript, 2> Pair(Pairs);
+  SmallVector<Subscript> Pair(Pairs);
   const SCEV *SrcSCEV = SE->getSCEV(SrcPtr);
----------------
Consider removing unrelated changes from this patch.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3615
 
+  // establish loop nesting levels
+  establishNestingLevels(Src, Dst, Pair);
----------------
Is there a reason this is moved?


================
Comment at: llvm/test/Analysis/DependenceAnalysis/MismatchingNestLevels.ll:11
+
+define dso_local void @test1([512 x double]* %temp) local_unnamed_addr {
+entry:
----------------
Could you add some pseudocode helping to understand the case?


================
Comment at: llvm/test/Analysis/DependenceAnalysis/MismatchingNestLevels.ll:24
+  %src = load double, double* %arrayidx295, align 8
+  br i1 undef, label %for.body272, label %for.body6
+
----------------
It would be better to not rely on `undef` in test cases. Handling of `undef` can change, depending on what an analysis thinks runs faster. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110972