Re: [parloops, PR83126], Use cached affine_ivs canonicalize_loop_ivs

["Bin.Cheng" <amker.cheng@gmail.com>]

On Wed, Mar 21, 2018 at 3:06 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 03/12/2018 01:14 PM, Richard Biener wrote:
>>
>> On Thu, 22 Feb 2018, Tom de Vries wrote:
>>
>>> Hi,
>>>
>>> this patch fixes an ICE in the parloops pass.
>>>
>>> The ICE (when compiling the test-case in attached patch) follows from the
>>> fact
>>> that here in gen_parallel_loop the call to canonicalize_loop_ivs fails to
>>> "base all the induction variables in LOOP on a single control one":
>>> ...
>>>    /* Base all the induction variables in LOOP on a single control one.*/
>>>    canonicalize_loop_ivs (loop, &nit, true);
>>> ...
>>>
>>> This is caused by the fact that for one of the induction variables,
>>> simple_iv
>>> no longer returns true (as was the case at the start of
>>> gen_parallel_loop).
>>> This seems to be triggered by the fact that during loop_version we call
>>> scev_reset (although I'm not sure why when recalculating scev info we're
>>> not
>>> reaching the same conclusion as before).
>>
>> I guess the real reason is that canonicalize_loop_ivs calls
>> create_iv first which in the parloop case with bump-in-latch true
>> and then incrementally re-writes PHIs, eventually wrecking other
>> PHIs SCEV.  In this case the 2nd PHIs evolution depends on the
>> first one but the rewritten ones depend on the new IV already.
>>
>> So the better fix (maybe not 100% enough) would be to re-organize
>> canonicalize_loop_ivs to first do analysis of all PHIs and then
>> rewrite them all.
>>
>
> Focusing on the re-organize first, is this sort of what you had in mind?

FYI, we have multiple interfaces manipulating on IVs, sometimes in a similar
way.  An example would be create_canonical_iv in tree-ssa-loop-ivcanon.c.

One proposal for discussion is to refactor such interfaces and put them in a
single source file, like tree-ssa-loop-ivopts.  For example of
create_canonical_iv,
it can be removed by simply adding the corresponding candidate in IVOPTs.

Thanks,
bin
>
> Bootstrapped and reg-tested on x86_64.
>
> Thanks,
> - Tom