Fix PR71824

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 <sebpop@gmail.com> wrote:
> > >
> > >
> > > On Tue, Jan 31, 2017 at 7:49 AM, Richard Biener <rguenther@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)>}

 

seems to fix it.

Richard.

On Tue, Jan 31, 2017 at 9:11 AM, Richard Biener <rguenther@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 <sebpop@gmail.com> wrote:
>> > >
>> > >
>> > > On Tue, Jan 31, 2017 at 7:49 AM, Richard Biener <rguenther@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.