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

[Arnold Schwaighofer]

r201465

Thank you Andy.

On Feb 14, 2014, at 8:05 PM, Andrew Trick <atrick at apple.com> wrote:

> 
> On Feb 14, 2014, at 3:41 PM, Arnold Schwaighofer <aschwaighofer at apple.com> wrote:
> 
>> 
>> On Feb 13, 2014, at 2:31 PM, Andrew Trick <atrick at apple.com> wrote:
>>> 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
>> 
>> Thanks for the review.
>> 
>> Changed so that we have one loop:
>> 
>> for ‘phi' in ‘loop' of expanded addrec ’normalize':
>> ismatchingscev = scev(phi) == scev(normalize)
>> 
>> // bail out early for non-exact-matching scevs if we don’t know the loop we are expanding dominates the loop we are expanding into.
>> if (!ismatchingscev)
>>   if(!IVIncInsertLoop ||
>>      properlyDominates(loop, IVIncInsertLoop)
>> 
>> // check whether we can reuse this phi
>> if(lsrmode)
>>   // check expandedaddrecphi
>>   // make sure we can hoist the increment above the insert pos
>> else
>>   // check isnormaladdrec
>> 
>> if (ismatchingscev)
>>   //sucess break out of loop.
>> otherwise:
>>   // check whether we can use the phi by truncating and/or inverting.
>> 
>> 
>> if (found match)
>> // hoist increment above insertpos if required
>> return the phi
> 
> Great. That's much easier to understand than the original code was.
> 
> As a follow up we should generalize this functionality so it works the same way for all SCEVExpander users, then remove any unnecessary special cases.
> 
> For example, I don't understand why the vectorizer uses "canonical" mode. Ideally we would force all SCEVExpander users to indicate the current loop being optimized (IVInsertLoop). Your logic for reusing IVs outside the current loop is applicable to all SCEVExpander passes.
> 
> It's ok to have some modes for heuristics. e.g. LSRMode avoids phis in the current loop that don't look like they were genererated by LSR. But the main logic and correctness should be fully general.
> 
> -Andy
> 
>> 
>> 
>> 
>> 
>> <0001-SCEVExpander-Try-hard-not-to-create-derived-inductio.patch>