[PATCH] D12765: [LV] Allow vectorization of loops with induction post-inc expressions

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi Jakub,

I also spent some time in debugger today, and now I'm convinced even more that we should try to handle it in IndVarSimplify/SCEV. As you correctly noted, the problem is that IndVarSimplify thinks that //smax// expression is expensive to expand. While it's indeed expensive to expand //smax/smin//, we don't need to do that in our case, as this particular expansion already exists in our code - variable `%n` holds this value. Moreover, SCEV already tries to find existing expansion (see `SCEVExpander::findExistingExpansion`), but fails in our case. The issue we should be solving here is how to make SCEVExpander not fail.

Now, how does it try to find existing expansion, and why does it fail? The logic is pretty simple here: we examine all checks in exiting blocks of our loop - `LHS` and `RHS` of these comparisons already have existing expansions by definition (they are the expansions). If their SCEV expression happens to be exactly the same as the one we're looking for (`S`) - we're done! But here is where our problem resides. In our case we are looking for the following expression:

  (lldb) p S->dump()
  (-3 + (-1 * (-4097 smax (-1 + (-1 * %y))))<nsw>)

But the most similar one that we have is this one:

  (lldb) p SE.getSCEV(RHS)->dump()
  (-2 + (-1 * (-4097 smax (-1 + (-1 * %y))))<nsw>)

So, we fail to see that we have almost the same existing one, so we think that we need to expand it from scratch - and that's expensive, so we bail out. Please note though that if we for some reason rewrote this expression as `(1+ -3 + (-1 * (-4097 smax (-1 + (-1 * %y))))<nsw>)`, the current logic would catch it.

With that in mind, I tried the following patch:

  diff --git a/lib/Analysis/ScalarEvolutionExpander.cpp b/lib/Analysis/ScalarEvolutionExpander.cpp
  index ed7386b..e4af9dc 100644
  --- a/lib/Analysis/ScalarEvolutionExpander.cpp
  +++ b/lib/Analysis/ScalarEvolutionExpander.cpp
  @@ -1829,10 +1829,16 @@ Value *SCEVExpander::findExistingExpansion(const SCEV *S,
                       TrueBB, FalseBB)))
         continue;
  
  -    if (SE.getSCEV(LHS) == S && SE.DT.dominates(LHS, At))
  +    const SCEV *LHSSE = SE.getSCEV(LHS);
  +    if (LHSSE->getType() == S->getType() &&
  +        isa<SCEVConstant>(SE.getMinusSCEV(LHSSE, S)) &&
  +        SE.DT.dominates(LHS, At))
         return LHS;
  
  -    if (SE.getSCEV(RHS) == S && SE.DT.dominates(RHS, At))
  +    const SCEV *RHSSE = SE.getSCEV(RHS);
  +    if (RHSSE->getType() == S->getType() &&
  +        isa<SCEVConstant>(SE.getMinusSCEV(RHSSE, S)) &&
  +        SE.DT.dominates(RHS, At))
         return RHS;
     }

The idea is that when an expression only differs by a constant from an existing one, we can say that it also exists (to be precise, there is a cheap way to expand it). However, I think it might be incorrect in some corner cases, so I'd like to check it with SCEV experts first. But anyway, even if it's not correct in this particular form, I think it should be possible to make it correct if we add some sanity checks, and I believe that's the way to go.

Thanks,
Michael

PS: I haven't done any testing of this patch except on the provided test case, which was successfully vectorized.


Repository:
  rL LLVM

http://reviews.llvm.org/D12765