| Message ID | 20120430133926.GL16117@tyan-ft48-01.lab.bos.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Apr 30, 2012 at 3:39 PM, Jakub Jelinek <jakub@redhat.com> wrote: > Hi! > > compute_all_dependences in 4.7+ can fail, and cond_if_else_store_replacement > isn't prepared to handle the chrec_dont_know DDR added there in case of > failure (with NULL DDR_A/DDR_B). > > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, > ok for trunk/4.7? Ok. > BTW, tree-ssa-loop-prefetch.c seems to have the same problem, but no idea > how that should be handled in there... I think it handles it fine by treating the chrec_dont_know DDR properly? I suppose failing would be an option, too, by returning a bool from determine_loop_nest_reuse and adjusting its single caller. Thanks, Richard. > 2012-04-30 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/53163 > * tree-ssa-phiopt.c (cond_if_else_store_replacement): Don't ignore > return value from compute_all_dependences. > > * gcc.c-torture/compile/pr53163.c: New test. > > --- gcc/tree-ssa-phiopt.c.jj 2012-04-30 08:06:18.000000000 +0200 > +++ gcc/tree-ssa-phiopt.c 2012-04-30 08:59:16.726488101 +0200 > @@ -1624,8 +1624,17 @@ cond_if_else_store_replacement (basic_bl > /* Compute and check data dependencies in both basic blocks. */ > then_ddrs = VEC_alloc (ddr_p, heap, 1); > else_ddrs = VEC_alloc (ddr_p, heap, 1); > - compute_all_dependences (then_datarefs, &then_ddrs, NULL, false); > - compute_all_dependences (else_datarefs, &else_ddrs, NULL, false); > + if (!compute_all_dependences (then_datarefs, &then_ddrs, NULL, false) > + || !compute_all_dependences (else_datarefs, &else_ddrs, NULL, false)) > + { > + free_dependence_relations (then_ddrs); > + free_dependence_relations (else_ddrs); > + free_data_refs (then_datarefs); > + free_data_refs (else_datarefs); > + VEC_free (gimple, heap, then_stores); > + VEC_free (gimple, heap, else_stores); > + return false; > + } > blocks[0] = then_bb; > blocks[1] = else_bb; > blocks[2] = join_bb; > --- gcc/testsuite/gcc.c-torture/compile/pr53163.c.jj 2012-04-30 09:21:40.950512562 +0200 > +++ gcc/testsuite/gcc.c-torture/compile/pr53163.c 2012-04-30 09:21:20.000000000 +0200 > @@ -0,0 +1,34 @@ > +/* PR tree-optimization/53163 */ > + > +struct S { int s; } b, f; > +int a, c; > + > +void > +foo (void) > +{ > + int d, e; > + for (d = 4; d < 19; ++d) > + for (e = 2; e >= 0; e--) > + { > + a = 0; > + a = 1; > + } > +} > + > +void > +bar (void) > +{ > + int g, h, i; > + for (i = 1; i >= 0; i--) > + { > + b = f; > + for (g = 0; g <= 1; g++) > + { > + if (c) > + break; > + for (h = 0; h <= 1; h++) > + foo (); > + foo (); > + } > + } > +} > > Jakub
On Wed, May 02, 2012 at 11:24:13AM +0200, Richard Guenther wrote: > > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, > > ok for trunk/4.7? > > Ok. Thanks. > > BTW, tree-ssa-loop-prefetch.c seems to have the same problem, but no idea > > how that should be handled in there... > > I think it handles it fine by treating the chrec_dont_know DDR > properly? I suppose > failing would be an option, too, by returning a bool from > determine_loop_nest_reuse > and adjusting its single caller. IMHO it will just segfault (I don't have a testcase though). compute_all_dependences (datarefs, &dependences, vloops, true); FOR_EACH_VEC_ELT (ddr_p, dependences, i, dep) { if (DDR_ARE_DEPENDENT (dep) == chrec_known) continue; ref = (struct mem_ref *) DDR_A (dep)->aux; refb = (struct mem_ref *) DDR_B (dep)->aux; if (DDR_ARE_DEPENDENT (dep) == chrec_dont_know || DDR_NUM_DIST_VECTS (dep) == 0) { /* If the dependence cannot be analyzed, assume that there might * be a reuse. */ dist = 0; ref->independent_p = false; refb->independent_p = false; } If compute_all_dependences above fails (returns false), then dependences vector will contain just single chrec_dont_know element, but with DDR_A (dep) == DDR_B (dep) == NULL. So the above will try to dereference both and ICE before checking chrec_dont_know (and even if it wouldn't, there is nothing to mark independent_p = false - supposedly everything should be no longer independent_p). Jakub
On Wed, May 2, 2012 at 12:04 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, May 02, 2012 at 11:24:13AM +0200, Richard Guenther wrote: >> > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, >> > ok for trunk/4.7? >> >> Ok. > > Thanks. > >> > BTW, tree-ssa-loop-prefetch.c seems to have the same problem, but no idea >> > how that should be handled in there... >> >> I think it handles it fine by treating the chrec_dont_know DDR >> properly? I suppose >> failing would be an option, too, by returning a bool from >> determine_loop_nest_reuse >> and adjusting its single caller. > > IMHO it will just segfault (I don't have a testcase though). > > compute_all_dependences (datarefs, &dependences, vloops, true); > > FOR_EACH_VEC_ELT (ddr_p, dependences, i, dep) > { > if (DDR_ARE_DEPENDENT (dep) == chrec_known) > continue; > > ref = (struct mem_ref *) DDR_A (dep)->aux; > refb = (struct mem_ref *) DDR_B (dep)->aux; > > if (DDR_ARE_DEPENDENT (dep) == chrec_dont_know > || DDR_NUM_DIST_VECTS (dep) == 0) > { > /* If the dependence cannot be analyzed, assume that there might > * be > a reuse. */ > dist = 0; > > ref->independent_p = false; > refb->independent_p = false; > } > > If compute_all_dependences above fails (returns false), then dependences > vector will contain just single chrec_dont_know element, but with DDR_A > (dep) == DDR_B (dep) == NULL. So the above will try to dereference both and > ICE before checking chrec_dont_know (and even if it wouldn't, there is > nothing to mark independent_p = false - supposedly everything should be > no longer independent_p). Hm, indeed. Mind fixing it the way I suggested? Thanks, Richard. > Jakub
--- gcc/tree-ssa-phiopt.c.jj 2012-04-30 08:06:18.000000000 +0200 +++ gcc/tree-ssa-phiopt.c 2012-04-30 08:59:16.726488101 +0200 @@ -1624,8 +1624,17 @@ cond_if_else_store_replacement (basic_bl /* Compute and check data dependencies in both basic blocks. */ then_ddrs = VEC_alloc (ddr_p, heap, 1); else_ddrs = VEC_alloc (ddr_p, heap, 1); - compute_all_dependences (then_datarefs, &then_ddrs, NULL, false); - compute_all_dependences (else_datarefs, &else_ddrs, NULL, false); + if (!compute_all_dependences (then_datarefs, &then_ddrs, NULL, false) + || !compute_all_dependences (else_datarefs, &else_ddrs, NULL, false)) + { + free_dependence_relations (then_ddrs); + free_dependence_relations (else_ddrs); + free_data_refs (then_datarefs); + free_data_refs (else_datarefs); + VEC_free (gimple, heap, then_stores); + VEC_free (gimple, heap, else_stores); + return false; + } blocks[0] = then_bb; blocks[1] = else_bb; blocks[2] = join_bb; --- gcc/testsuite/gcc.c-torture/compile/pr53163.c.jj 2012-04-30 09:21:40.950512562 +0200 +++ gcc/testsuite/gcc.c-torture/compile/pr53163.c 2012-04-30 09:21:20.000000000 +0200 @@ -0,0 +1,34 @@ +/* PR tree-optimization/53163 */ + +struct S { int s; } b, f; +int a, c; + +void +foo (void) +{ + int d, e; + for (d = 4; d < 19; ++d) + for (e = 2; e >= 0; e--) + { + a = 0; + a = 1; + } +} + +void +bar (void) +{ + int g, h, i; + for (i = 1; i >= 0; i--) + { + b = f; + for (g = 0; g <= 1; g++) + { + if (c) + break; + for (h = 0; h <= 1; h++) + foo (); + foo (); + } + } +}