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

[Richard Biener <rguenther at suse dot de>]

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.

> 
> The patch fixes the ICE by caching the affine_ivs as calculated by simple_iv
> before calling loop_version, and reusing those in canonicalize_loop_ivs
> (instead of calling simple_iv again).
> 
> Bootstrapped and reg-tested on x86_64.
> 
> OK for stage1?

First of all please to not use std::map but instead our own hash_map,
the proper way of including <map> would be to define INCLUDE_MAP
before the system.h include.

But see above.

> I'm not sure if this is minimal/low-impact enough for stage4. If not, I can
> rework the bit of the patch that adds an assert after canonicalize_loop_ivs
> into a patch that aborts the optimization instead of ICE-ing.

I think that's something reasonable anyway.

Thanks,
Richard.

> Thanks,
> - Tom
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)