[PATCH] D110973: [DA] Handle mismatching loop levels by considering them non-linear

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

As mentioned I would experimented a bit more. Unfortunately I found more problems.

The following crashes even with this patch: F23108829: da-variant-crash.ll <https://reviews.llvm.org/F23108829>

  void foo(int n, double * restrict A, double * restrict B) {
      for (int i = 0; i < n; ++i) {
          int s = 0;
          for (; s*s < n*n; ++s) { } // incomputable AddRec
           for (int k = 0; k < n; ++k) 
              A[s] = 0; // Invariant in innermost loop
           
          A[i] = 1; // do create a dependency pair
      }
  }

because the the index of `A[s]` is invariant to its innermost surrounding loop `k` is invariant and hence passes the additional test.

Even more concerning is something like this, even though it does not crash directly:

     int s = 0;
     for (; s*s < n*n; ++s) ; // incomputable AddRec
  
  for (int i = 0; i < n; ++i) {
       for (int k = 0; k < n; ++k) 
           for (int j = 0; j < n; ++j) 
              A[s + k] = 0; // Invariant: j; Computable: k; Variant: s
       
      A[i] = 1; // do create a dependency pair
  }

ScalarEvolution's LoopDisposition test skips `s` because it is nested inside an AddRec that is invariant because its of an outer loop (`((8 * (sext i32 {{0,+,1}<nuw><nsw><%for.cond>,+,1}<nuw><%for.cond5> to i64))<nsw> + %A)`) and hence returns the entire thing as invariant even though it contains an SCEVAddRecExpr outside the loop nest. I again tend towards to scanning the SCEVExpr for illegal SCEVAddRecExpr. See [the only important remark].



================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:798-799
                                      const Loop *LoopNest) const {
   if (!LoopNest)
     return true;
   return SE->isLoopInvariant(Expression, LoopNest) &&
----------------
This seems in conflict with handling within ScalarEvolution:
```
    // Instructions are never considered invariant in the function body
    // (null loop) because they are defined within the "loop".
```

Suggested alternative comments/non-recursive implementation:
```
  // Outside any loop, an expression will not vary between executions (there is only one) and we consider it invariant. In contrast to ScalarEvolution::isLoopInvariant, we only consider expression evalution at a specific position, the array access, not accross the function.
  if (!LoopNest)
    return true;

  // If the expression is invariant in the outermost loop of the loop nest, it is invariant anywhere in the loop nest. 
 const  Loop * OutermostLoop = LoopNest;
  while (OutermostLoop->getParentLoop())
      OutermostLoop = OutermostLoop->getParentLoop();
  return SE->isLoopInvariant(Expression, OutermostLoop);
```


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:892
 
 // Examine the scev and return true iff it's linear.
 // Collect any loops mentioned in the set of "Loops".
----------------
We allow for non-zero Start value, hence it is affine, not linear.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:900
   const SCEV *Start = AddRec->getStart();
   const SCEV *Step = AddRec->getStepRecurrence(*SE);
   const SCEV *UB = SE->getBackedgeTakenCount(AddRec->getLoop());
----------------
[suggestion] `SCEVAddRecExpr` has a member `isAffine` that could be queried directly.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:921-924
   if (IsSrc)
     Loops.set(mapSrcLoop(AddRec->getLoop()));
   else
     Loops.set(mapDstLoop(AddRec->getLoop()));
----------------
`mapSrcLoop`/`mapDstLoop` may already be out-of-range because `AddRec->getLoop()` is not surrounding the SCEVExpr. 

One could include a sanity check inside `mapSrcLoop`/`mapDstLoop`:
```
    const Loop* L = LoopNest;
    while (L && AddRec->getLoop() != L) 
        L = L->getParentLoop();
    assert (L && "Must be in loop nest, otherwise mapSrcLoop/mapDstLoop will be out-of-range");
```


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:925
     Loops.set(mapDstLoop(AddRec->getLoop()));
   return checkSubscript(Start, LoopNest, Loops, IsSrc);
 }
----------------
This recursive call may also return false, in which case `Loops.set` has already been executed. (This may be irrelevant because when returning false, `Loops` is probably be ignored anyway).


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:917
+  DispositionTy Disposition = SE->getLoopDisposition(AddRec, LoopNest);
+  if (Disposition != DispositionTy::LoopInvariant &&
+      Disposition != DispositionTy::LoopComputable)
----------------
congzhe wrote:
> bmahjour wrote:
> > congzhe wrote:
> > > congzhe wrote:
> > > > Thanks for the change, It looks good to me. I don't know if Micheal has further comments?
> > > > 
> > > > [nit] I'm wondering if it would be better to simplify the code like this?
> > > > ```
> > > > if (SE->getLoopDisposition(AddRec, LoopNest) == ScalarEvolution::LoopDisposition::LoopVariant )
> > > >   return false;
> > > > ```
> > > > Thanks for the change, It looks good to me. I don't know if Micheal has further comments?
> > > > 
> > > > [nit] I'm wondering if it would be better to simplify the code like this?
> > > > ```
> > > > if (SE->getLoopDisposition(AddRec, LoopNest) == ScalarEvolution::LoopDisposition::LoopVariant )
> > > >   return false;
> > > > ```
> > > 
> > > Actually could be futher simplified to 
> > > ```
> > > if (SE->getLoopDisposition(AddRec, LoopNest) == ScalarEvolution::LoopVariant )
> > >   return false;
> > > ```
> > > 
> > I thought about that too, but decided to check for `LoopComputable` and `LoopInvariant` specifically, in case new enums are added to `LoopDisposition` in the future. The logic also makes a bit more sense when I think about it as neither invariant nor computable. LoopVariant isn't the best wording for what it really means.
> Sure, makes sense to me.
[the only important remark] I suggest to use this check instead to fix the additional crash:
```
    // The AddRec must depend on one of the surrounding loops. Otherwise, mapSrcLoop and mapDstLoop return indices outside the intended range. This can happen when a subscript in one loop references an IV from a sibling loop that could not be replaced with a concrete exit value by getSCEVAtScope. 
    const Loop* L = LoopNest;
    while (L && AddRec->getLoop() != L) 
        L = L->getParentLoop();
    if (!L)
        return false;
```
Currently I am too unsure whether we can make computeLoopDisposition work reliably.

This ignores that there could be `SCEVAddRecExpr` in other SCEV subexpressions, but I think DA doesn't try to reach them anyway. Maybe add a comment about this somewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110973