| Message ID | alpine.LSU.2.20.1701311446160.12993@r111.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
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? > > Thanks, > Sebastian >
On Tue, 31 Jan 2017, Sebastian Pop 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)>} Richard. > Thanks, > Sebastian > > > > Bootstrap & regtest in progress (though the patch can at most turn > > an ICE into wrong-code). > > > > This fixes build of 445.gobmk with -floop-nest-optimize. > > > > Ok? > > > > Thanks, > > Richard > > > > 2017-01-31 Richard Biener <rguenther@suse.de> > > > > PR tree-optimization/71824 > > * graphite-sese-to-poly.c (add_loop_constraints): Bail out > > instead of asserting. > > > > * gcc.dg/graphite/pr71824.c: New testcase. > > > > Index: gcc/graphite-sese-to-poly.c > > =================================================================== > > --- gcc/graphite-sese-to-poly.c (revision 245058) > > +++ gcc/graphite-sese-to-poly.c (working copy) > > @@ -930,7 +931,11 @@ add_loop_constraints (scop_p scop, __isl > > /* loop_i <= expr_nb_iters */ > > gcc_assert (!chrec_contains_undetermined (nb_iters)); > > nb_iters = scalar_evolution_in_region (region, loop, nb_iters); > > - gcc_assert (!chrec_contains_undetermined (nb_iters)); > > + if (chrec_contains_undetermined (nb_iters)) > > + { > > + isl_space_free (space); > > + return domain; > > + } > > > > isl_pw_aff *aff_nb_iters = extract_affine (scop, nb_iters, > > isl_space_copy (space)); > > Index: gcc/testsuite/gcc.dg/graphite/pr71824.c > > =================================================================== > > --- gcc/testsuite/gcc.dg/graphite/pr71824.c (nonexistent) > > +++ gcc/testsuite/gcc.dg/graphite/pr71824.c (working copy) > > @@ -0,0 +1,17 @@ > > +/* { dg-do compile } */ > > +/* { dg-options "-O2 -floop-nest-optimize" } */ > > + > > +int a, b, d; > > +int **c; > > +int fn1() { > > + while (a) > > + if (d) { > > + int e = -d; > > + for (; b < e; b++) > > + c[b] = &a; > > + } else { > > + for (; b; b++) > > + c[b] = &b; > > + d = 0; > > + } > > +} > > >
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)>} Richard.
On Tue, Jan 31, 2017 at 9:06 AM, Richard Biener <rguenther@suse.de> wrote: > On Tue, 31 Jan 2017, Sebastian Pop 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? We are supposed to verify that the merged regions are still a valid scop. So we are probably missing in merge_sese an extra call to verify that all the loops can be represented in the combined region.
Index: gcc/graphite-sese-to-poly.c =================================================================== --- gcc/graphite-sese-to-poly.c (revision 245058) +++ gcc/graphite-sese-to-poly.c (working copy) @@ -930,7 +931,11 @@ add_loop_constraints (scop_p scop, __isl /* loop_i <= expr_nb_iters */ gcc_assert (!chrec_contains_undetermined (nb_iters)); nb_iters = scalar_evolution_in_region (region, loop, nb_iters); - gcc_assert (!chrec_contains_undetermined (nb_iters)); + if (chrec_contains_undetermined (nb_iters)) + { + isl_space_free (space); + return domain; + } isl_pw_aff *aff_nb_iters = extract_affine (scop, nb_iters, isl_space_copy (space)); Index: gcc/testsuite/gcc.dg/graphite/pr71824.c =================================================================== --- gcc/testsuite/gcc.dg/graphite/pr71824.c (nonexistent) +++ gcc/testsuite/gcc.dg/graphite/pr71824.c (working copy) @@ -0,0 +1,17 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -floop-nest-optimize" } */ + +int a, b, d; +int **c; +int fn1() { + while (a) + if (d) { + int e = -d; + for (; b < e; b++) + c[b] = &a; + } else { + for (; b; b++) + c[b] = &b; + d = 0; + } +}