| Message ID | 20151208162133.GQ3175@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/12/15 03:21, Marek Polacek wrote: > The following is a conservative fix for this PR. This is an ICE transpiring > in the new "Factor conversion in COND_EXPR" optimization added in r225722. > > Before this optimization kicks in, we have > <bb 2>: > ... > p1_32 = (short unsigned int) _20; > > <bb 3>: > ... > iftmp.0_18 = (short unsigned int) _20; > > <bb 4>: > ... > # iftmp.0_19 = PHI <iftmp.0_18(3), p1_32(2)> > > after factor_out_conditional_conversion does its work, we end up with those two > def stmts removed and instead of the PHI we'll have > > # _35 = PHI <_20(3), _20(2)> > iftmp.0_19 = (short unsigned int) _35; > > That itself looks like a fine optimization, but after factor_out_conditional_conversion > there's > 320 phis = phi_nodes (bb2); > 321 phi = single_non_singleton_phi_for_edges (phis, e1, e2); > 322 gcc_assert (phi); > and phis look like > b.2_38 = PHI <b.2_9(3), b.2_9(2)> > _35 = PHI <_20(3), _20(2)> > so single_non_singleton_phi_for_edges returns NULL and the subsequent assert triggers. > > With this patch we won't ICE (and PRE should clean this up anyway), but I don't know, > maybe I should try harder to optimize even this problematical case (not sure how hard > it would be...)? Hi Marek, Thanks for fixing this. Yes, we can try remove the PHI in factor_out_conditional_conversion. But as you said, it might not be important. In any case, please let me know if you want me to try doing that. Thanks, Kugan > > pr66949-2.c only ICEd on powerpc64le and I have verified that this patch fixes it too. > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2015-12-08 Marek Polacek <polacek@redhat.com> > > PR tree-optimization/66949 > * tree-ssa-phiopt.c (factor_out_conditional_conversion): Return false if > NEW_ARG0 and NEW_ARG1 are equal. > > * gcc.dg/torture/pr66949-1.c: New test. > * gcc.dg/torture/pr66949-2.c: New test. > > diff --git gcc/testsuite/gcc.dg/torture/pr66949-1.c gcc/testsuite/gcc.dg/torture/pr66949-1.c > index e69de29..1b765bc 100644 > --- gcc/testsuite/gcc.dg/torture/pr66949-1.c > +++ gcc/testsuite/gcc.dg/torture/pr66949-1.c > @@ -0,0 +1,28 @@ > +/* PR tree-optimization/66949 */ > +/* { dg-do compile } */ > + > +int a, *b = &a, c; > + > +unsigned short > +fn1 (unsigned short p1, unsigned int p2) > +{ > + return p2 > 1 || p1 >> p2 ? p1 : p1 << p2; > +} > + > +void > +fn2 () > +{ > + int *d = &a; > + for (a = 0; a < -1; a = 1) > + ; > + if (a < 0) > + c = 0; > + *b = fn1 (*d || c, *d); > +} > + > +int > +main () > +{ > + fn2 (); > + return 0; > +} > diff --git gcc/testsuite/gcc.dg/torture/pr66949-2.c gcc/testsuite/gcc.dg/torture/pr66949-2.c > index e69de29..e6250a3 100644 > --- gcc/testsuite/gcc.dg/torture/pr66949-2.c > +++ gcc/testsuite/gcc.dg/torture/pr66949-2.c > @@ -0,0 +1,23 @@ > +/* PR tree-optimization/66949 */ > +/* { dg-do compile } */ > + > +char a; > +int b, c, d; > +extern int fn2 (void); > + > +short > +fn1 (short p1, short p2) > +{ > + return p2 == 0 ? p1 : p1 / p2; > +} > + > +int > +main (void) > +{ > + char e = 1; > + int f = 7; > + c = a >> f; > + b = fn1 (c, 0 < d <= e && fn2 ()); > + > + return 0; > +} > diff --git gcc/tree-ssa-phiopt.c gcc/tree-ssa-phiopt.c > index 344cd2f..caac5d5 100644 > --- gcc/tree-ssa-phiopt.c > +++ gcc/tree-ssa-phiopt.c > @@ -477,6 +477,11 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi, > return false; > } > > + /* If we were to continue, we'd create a PHI with same arguments for edges > + E0 and E1. That could get us in trouble later, so punt. */ > + if (operand_equal_for_phi_arg_p (new_arg0, new_arg1)) > + return false; > + > /* If arg0/arg1 have > 1 use, then this transformation actually increases > the number of expressions evaluated at runtime. */ > if (!has_single_use (arg0) > > Marek >
On Tue, Dec 8, 2015 at 5:21 PM, Marek Polacek <polacek@redhat.com> wrote: > The following is a conservative fix for this PR. This is an ICE transpiring > in the new "Factor conversion in COND_EXPR" optimization added in r225722. > > Before this optimization kicks in, we have > <bb 2>: > ... > p1_32 = (short unsigned int) _20; > > <bb 3>: > ... > iftmp.0_18 = (short unsigned int) _20; > > <bb 4>: > ... > # iftmp.0_19 = PHI <iftmp.0_18(3), p1_32(2)> > > after factor_out_conditional_conversion does its work, we end up with those two > def stmts removed and instead of the PHI we'll have > > # _35 = PHI <_20(3), _20(2)> > iftmp.0_19 = (short unsigned int) _35; > > That itself looks like a fine optimization, but after factor_out_conditional_conversion > there's > 320 phis = phi_nodes (bb2); > 321 phi = single_non_singleton_phi_for_edges (phis, e1, e2); > 322 gcc_assert (phi); > and phis look like > b.2_38 = PHI <b.2_9(3), b.2_9(2)> > _35 = PHI <_20(3), _20(2)> > so single_non_singleton_phi_for_edges returns NULL and the subsequent assert triggers. > > With this patch we won't ICE (and PRE should clean this up anyway), but I don't know, > maybe I should try harder to optimize even this problematical case (not sure how hard > it would be...)? > > pr66949-2.c only ICEd on powerpc64le and I have verified that this patch fixes it too. > > Bootstrapped/regtested on x86_64-linux, ok for trunk? Hmm. Yes, there is an opportunity for optimization. But I don't see why we have the asserts at all - they seem to be originated from development. IMHO factor_out_conditonal_conversion should simply return the new PHI it generates instead of relying on signle_non_signelton_phi_for_edges to return it. Richard. > 2015-12-08 Marek Polacek <polacek@redhat.com> > > PR tree-optimization/66949 > * tree-ssa-phiopt.c (factor_out_conditional_conversion): Return false if > NEW_ARG0 and NEW_ARG1 are equal. > > * gcc.dg/torture/pr66949-1.c: New test. > * gcc.dg/torture/pr66949-2.c: New test. > > diff --git gcc/testsuite/gcc.dg/torture/pr66949-1.c gcc/testsuite/gcc.dg/torture/pr66949-1.c > index e69de29..1b765bc 100644 > --- gcc/testsuite/gcc.dg/torture/pr66949-1.c > +++ gcc/testsuite/gcc.dg/torture/pr66949-1.c > @@ -0,0 +1,28 @@ > +/* PR tree-optimization/66949 */ > +/* { dg-do compile } */ > + > +int a, *b = &a, c; > + > +unsigned short > +fn1 (unsigned short p1, unsigned int p2) > +{ > + return p2 > 1 || p1 >> p2 ? p1 : p1 << p2; > +} > + > +void > +fn2 () > +{ > + int *d = &a; > + for (a = 0; a < -1; a = 1) > + ; > + if (a < 0) > + c = 0; > + *b = fn1 (*d || c, *d); > +} > + > +int > +main () > +{ > + fn2 (); > + return 0; > +} > diff --git gcc/testsuite/gcc.dg/torture/pr66949-2.c gcc/testsuite/gcc.dg/torture/pr66949-2.c > index e69de29..e6250a3 100644 > --- gcc/testsuite/gcc.dg/torture/pr66949-2.c > +++ gcc/testsuite/gcc.dg/torture/pr66949-2.c > @@ -0,0 +1,23 @@ > +/* PR tree-optimization/66949 */ > +/* { dg-do compile } */ > + > +char a; > +int b, c, d; > +extern int fn2 (void); > + > +short > +fn1 (short p1, short p2) > +{ > + return p2 == 0 ? p1 : p1 / p2; > +} > + > +int > +main (void) > +{ > + char e = 1; > + int f = 7; > + c = a >> f; > + b = fn1 (c, 0 < d <= e && fn2 ()); > + > + return 0; > +} > diff --git gcc/tree-ssa-phiopt.c gcc/tree-ssa-phiopt.c > index 344cd2f..caac5d5 100644 > --- gcc/tree-ssa-phiopt.c > +++ gcc/tree-ssa-phiopt.c > @@ -477,6 +477,11 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi, > return false; > } > > + /* If we were to continue, we'd create a PHI with same arguments for edges > + E0 and E1. That could get us in trouble later, so punt. */ > + if (operand_equal_for_phi_arg_p (new_arg0, new_arg1)) > + return false; > + > /* If arg0/arg1 have > 1 use, then this transformation actually increases > the number of expressions evaluated at runtime. */ > if (!has_single_use (arg0) > > Marek
On 12/08/2015 09:21 AM, Marek Polacek wrote: > The following is a conservative fix for this PR. This is an ICE transpiring > in the new "Factor conversion in COND_EXPR" optimization added in r225722. > > Before this optimization kicks in, we have > <bb 2>: > ... > p1_32 = (short unsigned int) _20; > > <bb 3>: > ... > iftmp.0_18 = (short unsigned int) _20; > > <bb 4>: > ... > # iftmp.0_19 = PHI <iftmp.0_18(3), p1_32(2)> > > after factor_out_conditional_conversion does its work, we end up with those two > def stmts removed and instead of the PHI we'll have > > # _35 = PHI <_20(3), _20(2)> > iftmp.0_19 = (short unsigned int) _35; > > That itself looks like a fine optimization, but after factor_out_conditional_conversion > there's > 320 phis = phi_nodes (bb2); > 321 phi = single_non_singleton_phi_for_edges (phis, e1, e2); > 322 gcc_assert (phi); > and phis look like > b.2_38 = PHI <b.2_9(3), b.2_9(2)> > _35 = PHI <_20(3), _20(2)> > so single_non_singleton_phi_for_edges returns NULL and the subsequent assert triggers. Cute. > > With this patch we won't ICE (and PRE should clean this up anyway), but I don't know, > maybe I should try harder to optimize even this problematical case (not sure how hard > it would be...)? So if this kind of situation is created by the first phiopt, then DOM should fix it. As you note PRE will catch it if the second phiopt pass creates this degenerate PHI. If created by the last phiopt pass, then hopefully coalescing would save us. Jeff
diff --git gcc/testsuite/gcc.dg/torture/pr66949-1.c gcc/testsuite/gcc.dg/torture/pr66949-1.c index e69de29..1b765bc 100644 --- gcc/testsuite/gcc.dg/torture/pr66949-1.c +++ gcc/testsuite/gcc.dg/torture/pr66949-1.c @@ -0,0 +1,28 @@ +/* PR tree-optimization/66949 */ +/* { dg-do compile } */ + +int a, *b = &a, c; + +unsigned short +fn1 (unsigned short p1, unsigned int p2) +{ + return p2 > 1 || p1 >> p2 ? p1 : p1 << p2; +} + +void +fn2 () +{ + int *d = &a; + for (a = 0; a < -1; a = 1) + ; + if (a < 0) + c = 0; + *b = fn1 (*d || c, *d); +} + +int +main () +{ + fn2 (); + return 0; +} diff --git gcc/testsuite/gcc.dg/torture/pr66949-2.c gcc/testsuite/gcc.dg/torture/pr66949-2.c index e69de29..e6250a3 100644 --- gcc/testsuite/gcc.dg/torture/pr66949-2.c +++ gcc/testsuite/gcc.dg/torture/pr66949-2.c @@ -0,0 +1,23 @@ +/* PR tree-optimization/66949 */ +/* { dg-do compile } */ + +char a; +int b, c, d; +extern int fn2 (void); + +short +fn1 (short p1, short p2) +{ + return p2 == 0 ? p1 : p1 / p2; +} + +int +main (void) +{ + char e = 1; + int f = 7; + c = a >> f; + b = fn1 (c, 0 < d <= e && fn2 ()); + + return 0; +} diff --git gcc/tree-ssa-phiopt.c gcc/tree-ssa-phiopt.c index 344cd2f..caac5d5 100644 --- gcc/tree-ssa-phiopt.c +++ gcc/tree-ssa-phiopt.c @@ -477,6 +477,11 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi, return false; } + /* If we were to continue, we'd create a PHI with same arguments for edges + E0 and E1. That could get us in trouble later, so punt. */ + if (operand_equal_for_phi_arg_p (new_arg0, new_arg1)) + return false; + /* If arg0/arg1 have > 1 use, then this transformation actually increases the number of expressions evaluated at runtime. */ if (!has_single_use (arg0)