[PATCH] SCEV: Improve recognizing of consecutive loads/stores

[Michael Zolotukhin]

Hi Andy,

Thanks for the review! Indeed, all the checks that you spotted were redundant, and I removed them.

Here is an updated patch:

-------------- next part --------------
A non-text attachment was scrubbed...
Name: slp-consecutive-accesses-v2.patch
Type: application/octet-stream
Size: 10115 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140523/070f5d74/attachment.obj>
-------------- next part --------------


Doe it look ok?
I tested it on make check-all and benchmarks (for stability only).

Thanks,
Michael

On May 23, 2014, at 3:30 AM, Andrew Trick <atrick at apple.com> wrote:

> 
> On May 22, 2014, at 5:31 AM, Michael Zolotukhin <mzolotukhin at apple.com> wrote:
> 
>> Hi,
>> 
>> This patch teaches SCEV to better handle expressions with sign-extends. That will allow SLP-vectorizer to recognize consecutive memory accesses better.
>> 
>> *** Issue ***
>> Currently, ScalarEvolution fails to figure out that the following expressions describe consecutive loads/stores:
>> A = ((8 * (sext i32 (2 * %i) to i64)) + @a)
>> B = ((8 * (sext i32 (1 + (2 * %i)) to i64)) + @a)
>> 
>> Due to sign-ext presence in the example above, SE fails to simplify (B-A) to a constant.
>> 
>> *** Proposed solution ***
>> If we transform B expression to the following form, SE will be able to figure out that A and B are consecutive:
>> B' = ((8 * (1 + (sext i32 (2 * %i) to i64))) + @a) 
>> 
>> The current patch implements transformation (sext (A + B*X)) to (A + sext (B*X)). This is legal only if several conditions are true:
>> 1) A and B are positive constants (this could be easily extended to negative)
>> 2) B is a power of 2
>> 3) A < B
>> 
>> Similar transformation is applicable for AddRec expressions: sext{A,+,B} —> A + sext{0,+,B}. However, it’s worth noticing that canonicalization of AddRec expressions tends to accumulate all constants into the initial value, while this transformation tries to do the opposite - hoist the initial value out of AddRec.
>> 
>> I tested this patch on benchmarks, but saw nothing but noise (there were several degradations and improvements, but all of them disappeared after rerun).
>> 
>> Is this patch ok for trunk?
> 
> Hi Michael,
> 
> This looks great to me, I’m just not sure you need to check so many cases.
> 
> Is this really necessary? Shouldn’t GroupByComplexity canonicalize the AddExpr for you?
> 
> +      if (isa<SCEVConstant>(Op2)) {
> +        auto T = Op1;
> +        Op1 = Op2;
> +        Op2 = T;
> +      }
> 
> Similarly, shouldn’t a zero AddExpr operand have been erased?
> 
> +      if (SMul && SC1 && !SC1->isZero()) {
> 
> Since SCEVConstant is the simplest type, you probably don’t need a loop. Just check the first operand.
> 
> +        for (SCEVNAryExpr::op_iterator I = SMul->op_begin(), E = SMul->op_end();
> +             I != E; ++I) {
> +          if (auto C = dyn_cast<SCEVConstant>(*I)) {
> 
> This comment was copy-pasted. It could be rewritten as a recurrence:
> 
> +      // sext(C1 + (C2 * x)) --> C1 + sext(C2 * x) if C1 < C2
> 
> Out of curiosity, are there cases in which Start->getType() == Ty?
> 
> +          if (Start->getType() != Ty)
> +            Start = getSignExtendExpr(Start, Ty);
> 
> You’re dropping the no-wrap flags here. Since you we’re coming from a sign-extend, we probably won’t have NSW anyway, but it’s still good to preserve the flags.
> 
> +          const SCEV *NewAR = getAddRecExpr(getConstant(AR->getType(), 0), Step,
> +                                            L, SCEV::FlagAnyWrap);
> 
> Since you’re just shifting the recurrence, you can trivially preserve the NW flag:
> 
> AR->getNoWrapFlags(SCEV::FlagNW)
> 
> But since you’ve checked that both constants are positive, and you’re shifting the start to zero, you can preserve signed and unsigned wrap too. That means you can just preserve all the flags like this:
> 
> AR->getNoWrapFlags()
> 
> -Andy