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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added a comment.

In D110973#3492349 <https://reviews.llvm.org/D110973#3492349>, @Meinersbur wrote:

> I was expecting that we only need to call getLoopDisposition once at the beginning of `checkSubscript` and then decide depending on whether it is a SCEVAddRecExpr, instead of
>
>   if (!AddRec)
>     return isLoopInvariant(Expr, LoopNest);
>
> Is this possible?

I don't think we can do that, because if input is an AddRec that is invariant at the "LoopNest" level but variant at an outer level, we don't have a good way to detect it. This is related to your question about checking "all loops in the nest" (similar to isLoopInvariant), which we cannot do as explained below.

> Also, this patch only checks the innermost loop while `isLoopInvariant` checks all loops in the nest. Would we need as well?

When computing LoopDisposition of an AddRec at an outer loop that contains the AddRec's loop, the algorithm considers it `LoopVariant` due to the following logic in `computeLoopDisposition`:

  // Everything that is not defined at loop entry is variant.
  if (DT.dominates(L->getHeader(), AR->getLoop()->getHeader()))
    return LoopVariant;

For this reason it would not be correct to check LoopDisposition all the way from innermost to outermost. Doing so, would pessimistically classify subscripts that we care about the most. `isLoopInvariant` on the other hand is only called for non-AddRecs (which are less analyzable anyway) so it conservatively checks for all loops to make sure the subscript is considered linear only if it's invariant at each loop level.

> Would this fix miss the following case?
>
>   for (int i = ) {
>      int j = ...
>      for (j < ...; ++j ) {
>      }
>      for (int k = ) {
>         S[i][j][k];
>     }
>   }
>
> That is, j is invariant to the k-loop, put still contains a SCEVAddRecExpr of a loop outside the loop nest.

I don't think this is a problem, because for the level-mismatch problem to occur there must be another aliasing access in a different level outside of loop j and loop k (for example at i level). If we have an access at i level that has a recurrence with loop j, the disposition will be `LoopVariant` at i level. I couldn't come up with a test case for this due to the additional complication that if delinearization succeeds, the subscript being passed to `checkSubscript` has been computed with `SCEVAtScope`, so references to j at the i level would be replaced with j's loop exit value, thereby removing the problematic recurrence in the subscript.

> An alternative check:
>
>   diff --git a/llvm/lib/Analysis/DependenceAnalysis.cpp b/llvm/lib/Analysis/DependenceAnalysis.cpp
>   index 61bc6750277f7..4a3c5c5e4b202 100644
>   --- a/llvm/lib/Analysis/DependenceAnalysis.cpp
>   +++ b/llvm/lib/Analysis/DependenceAnalysis.cpp
>   @@ -891,6 +893,20 @@ void DependenceInfo::removeMatchingExtensions(Subscript *Pair) {
>    // Collect any loops mentioned in the set of "Loops".
>    bool DependenceInfo::checkSubscript(const SCEV *Expr, const Loop *LoopNest,
>                                        SmallBitVector &Loops, bool IsSrc) {
>   +  DenseSet<const Loop *> LoopsInNest;
>   +  const Loop *L = LoopNest;
>   +  while (L) {
>   +    LoopsInNest.insert(L);
>   +    L = L->getParentLoop();
>   +  }
>   +
>   +  if (SCEVExprContains(Expr, [&](const SCEV *E) -> bool {
>   +        if (auto *AddRec = dyn_cast<SCEVAddRecExpr>(E))
>   +          return !LoopsInNest.contains(AddRec->getLoop());
>   +        return false;
>   +      }))
>   +    return false;
>   +
>      const SCEVAddRecExpr *AddRec = dyn_cast<SCEVAddRecExpr>(Expr);
>      if (!AddRec)
>        return isLoopInvariant(Expr, LoopNest);
>
> This change passes all the DA tests, inlcuding `MismatchingNestLevels`
> I think (hope?) that such a loop will result in the SCEV loop disposition to be LoopDisposition::LoopVariant, so the getLoopDisposition solution should be better. However, it might not suffice if the loop is not in loop all:
>
>   int j = ...
>   for (j < ...; ++j ) {
>   }
>   S[j];
>
> Here, we don't have a loop that we could check the accesses' disposition for.

computeLoopDisposition returns `LoopVariant` for cases where the loop is nullptr (ie function scope), so it should be sufficient for this case too. However, your alternative makes it more obvious and easier to understand, so I'd be ok with that too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110973