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

[Mikhail Zolotukhin via llvm-commits]

Hi Andy, Sanjoy,

Do you mind taking a look at the following proposed change for SCEVExpander? The idea is to allow findExistingExpansion to find not only exactly matching expressions, but also expressions, that differ by a constant. My concern here is that while A==B isn't equivalent to getMinusSCEV(A,B)==Zero due to possible overflows (or is it?). If that's right, would it be possible to only apply this idea for cases when it's safe?

Thanks,
Michael
> On Sep 16, 2015, at 8:34 PM, Michael Zolotukhin <mzolotukhin at apple.com> wrote:
> 
> 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
> 
> 
>