[PATCH] Improve ScalarEvolutionExpander to try harder not to create unnecessary induction variables in dominating loops

[Andrew Trick]

On Feb 13, 2014, at 1:20 PM, Arnold Schwaighofer <aschwaighofer at apple.com> wrote:

> During LSR of one loop we would run into a situation where we had to expand the
> start of a recurrence of a loop induction variable in this loop. This start
> value was a value derived of the induction variable of a preceeding loop. SCEV
> has cannonicalized this value to a different recurrence than the recurrence of
> the preceeding loop's induction variable (the type and/or step direction) has
> changed). So when we come to instantiate this SCEV we would create a second
> induction variable in this preceeding loop.  This patch tries to base such
> derived induction variables of the preceeding loop's induction variable.
> 
> for ( ++i ) %for.cond38
> …
> 
> vectorized-for() %vector.body
> …
> 
> For example, we had the following SCEV for one of strength reduced induction variables:
> 
> {(1 + (zext i32 {(1 + (trunc i64 %smax to i32)),+,-1}<%for.cond38> to i64) + (-1 * (zext i3 {(1 + (trunc i64 (1 + %smax) to i
> 3)),+,-1}<%for.cond38> to i64))),+,-8}<%vector.body>
> 
> When we expanded {(1 + (trunc i64 (1 + %smax) to i3)),+,-1}<%for.cond38> we would create an unnecessary i3 induction variable in the “%for.cond” loop.
> 
> This helps twolf on arm and seems to help scimark2 on x86.


This has always been a problem with SCEVExpander, hence LSR. Your fix looks like a major improvement. Unfortunately, making SCEVExpander do reasonable things requires a lot of code complexity.

So, why is this gated by LSRMode? I think it should be on all the time.

Why is this separate from the existing search for reusable phis just above it? I think this is just another set of conditions similar to what we're already searching for. Can we combine the search but split logic into helpers? I think there are overlapping concerns between the existing logic and yours. It would be nice to see these issue cleanly separated:
- In LSRMode, if this is the current LSR loop, make sure it looks like an LSR-ish phi
- Make sure the expanded expression dominates it users (check isNormalAddRecExprPHI only when necessary)
- Find out of the phi's AddRec can be easily/cheaply converted into the one we're requesting.

-Andy