On Tue, Jan 31, 2017 at 9:11 AM, Richard Biener <rguent...@suse.de> wrote: > On Tue, 31 Jan 2017, Richard Biener wrote: > >> On Tue, 31 Jan 2017, Sebastian Pop wrote: >> >> > Resend as plain text to please gcc-patches@ >> > >> > On Tue, Jan 31, 2017 at 8:39 AM, Sebastian Pop <seb...@gmail.com> wrote: >> > > >> > > >> > > On Tue, Jan 31, 2017 at 7:49 AM, Richard Biener <rguent...@suse.de> >> > > wrote: >> > >> >> > >> >> > >> The following fixes an ICE that happens because instantiate_scev >> > >> doesn't really work as expected for SESE regions (a FIXME comment >> > >> hints at this already). So instead of asserting all goes well >> > >> just bail out (add_loop_constraints seems to add constraints not >> > >> required for correctness?). >> > > >> > > >> > > The conditions under which a loop is executed are required for >> > > correctness. >> > > There is a similar check in scop_detection::can_represent_loop_1 >> > > >> > > && (niter = number_of_latch_executions (loop)) >> > > && !chrec_contains_undetermined (niter) >> > > >> > > that is supposed to filter out all these loops where this assert does not >> > > hold. >> > > The question is: why scop detection has not rejected this loop? >> > > >> > > Well, I see that we do not check that niter can be analyzed in the >> > > region: >> > > so we would need another check like this: >> > > >> > > diff --git a/gcc/graphite-scop-detection.c >> > > b/gcc/graphite-scop-detection.c >> > > index 3860693..8e14412 100644 >> > > --- a/gcc/graphite-scop-detection.c >> > > +++ b/gcc/graphite-scop-detection.c >> > > @@ -931,6 +931,7 @@ scop_detection::can_represent_loop_1 (loop_p loop, >> > > sese_l scop) >> > > && niter_desc.control.no_overflow >> > > && (niter = number_of_latch_executions (loop)) >> > > && !chrec_contains_undetermined (niter) >> > > + && !chrec_contains_undetermined (scalar_evolution_in_region (scop, >> > > loop, niter)) >> > > && graphite_can_represent_expr (scop, loop, niter); >> > > } >> > > >> > > Could you please try this patch and see whether it fixes the problem? >> >> It doesn't. It seems we test the above before the regions are >> eventually merged? That is, the above enters with >> >> $46 = (const sese_l &) @0x7fffffffd6f0: { >> entry = <edge 0x7ffff68af508 (6 -> 7)>, >> exit = <edge 0x7ffff68af5b0 (7 -> 8)>} >> >> but the failing case with >> >> $15 = (const sese_l &) @0x298b420: {entry = <edge 0x7ffff68af738 (15 -> >> 3)>, >> exit = <edge 0x7ffff68af930 (14 -> 15)>} > > Index: graphite-scop-detection.c > =================================================================== > --- graphite-scop-detection.c (revision 245064) > +++ graphite-scop-detection.c (working copy) > @@ -905,7 +905,9 @@ scop_detection::build_scop_breadth (sese > > sese_l combined = merge_sese (s1, s2); > > - if (combined) > + if (combined > + && loop_is_valid_in_scop (loop, combined) > + && loop_is_valid_in_scop (loop->next, combined))
Looks good. Thanks for the fix! Sebastian > s1 = combined; > else > add_scop (s2); > @@ -931,6 +933,8 @@ scop_detection::can_represent_loop_1 (lo > && niter_desc.control.no_overflow > && (niter = number_of_latch_executions (loop)) > && !chrec_contains_undetermined (niter) > + && !chrec_contains_undetermined (scalar_evolution_in_region (scop, > + loop, > niter)) > && graphite_can_represent_expr (scop, loop, niter); > } > > > seems to fix it. > > Richard.