On Wed, Mar 21, 2018 at 3:06 PM, Tom de Vries <tom_devr...@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
