[PATCH] D33121: [SCEV] Fix getAddExpr for recurrencies from different loops

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added a comment.

Hi Max,

I'm not sure if such a large change is necessary.  Why not do something surgical like this:

  diff --git a/lib/Analysis/ScalarEvolution.cpp b/lib/Analysis/ScalarEvolution.cpp
  index d712063..054571f 100644
  --- a/lib/Analysis/ScalarEvolution.cpp
  +++ b/lib/Analysis/ScalarEvolution.cpp
  @@ -2448,6 +2448,15 @@ const SCEV *ScalarEvolution::getAddExpr(SmallVectorImpl<const SCEV *> &Ops,
       // Scan all of the other operands to this add and add them to the vector if
       // they are loop invariant w.r.t. the recurrence.
       SmallVector<const SCEV *, 8> LIOps;
  +    int BotIdx = Idx;
  +    for (int I = Idx + 1; I < Ops.size() && isa<SCEVAddRecExpr>(Ops[I]); ++I)
  +      if (DT.dominates(
  +              cast<SCEVAddRecExpr>(Ops[BotIdx])->getLoop()->getHeader(),
  +              cast<SCEVAddRecExpr>(Ops[I])->getLoop()->getHeader()))
  +        BotIdx = I;
  +
  +    std::swap(Ops[Idx], Ops[BotIdx]);
  +
       const SCEVAddRecExpr *AddRec = cast<SCEVAddRecExpr>(Ops[Idx]);
       const Loop *AddRecLoop = AddRec->getLoop();
       for (unsigned i = 0, e = Ops.size(); i != e; ++i)

that uses the `Ops` array as a scratch space to get the invariant you need?

The other option (that I like more) is to change `CompareSCEVComplexity` to get this invariant directly from `GroupByComplexity`.  After that, we can have an assert in `getAddExpr` to check that `GroupByComplexity` did what we wanted.


https://reviews.llvm.org/D33121