| Message ID | 20151209142223.GR3175@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Dec 9, 2015 at 3:22 PM, Marek Polacek <polacek@redhat.com> wrote: > On Wed, Dec 09, 2015 at 11:41:44AM +0100, Richard Biener wrote: >> 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. > > Ok, that seems to work. > > Bootstrapped/regtested on x86_64-linux, ok for trunk? Ok. Thanks, Richard. > 2015-12-09 Marek Polacek <polacek@redhat.com> > > PR tree-optimization/66949 > * tree-ssa-phiopt.c (tree_ssa_phiopt_worker): Don't call > single_non_singleton_phi_for_edges to get the PHI from > factor_out_conditional_conversion. Use NULL_TREE instead of NULL. > (factor_out_conditional_conversion): Adjust declaration. Make it > return the newly-created PHI. > > * 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..d7e8aa0 100644 > --- gcc/tree-ssa-phiopt.c > +++ gcc/tree-ssa-phiopt.c > @@ -49,7 +49,7 @@ along with GCC; see the file COPYING3. If not see > static unsigned int tree_ssa_phiopt_worker (bool, bool); > static bool conditional_replacement (basic_block, basic_block, > edge, edge, gphi *, tree, tree); > -static bool factor_out_conditional_conversion (edge, edge, gphi *, tree, tree); > +static gphi *factor_out_conditional_conversion (edge, edge, gphi *, tree, tree); > static int value_replacement (basic_block, basic_block, > edge, edge, gimple *, tree, tree); > static bool minmax_replacement (basic_block, basic_block, > @@ -310,19 +310,19 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads) > > /* Something is wrong if we cannot find the arguments in the PHI > node. */ > - gcc_assert (arg0 != NULL && arg1 != NULL); > + gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE); > > - if (factor_out_conditional_conversion (e1, e2, phi, arg0, arg1)) > + gphi *newphi = factor_out_conditional_conversion (e1, e2, phi, > + arg0, arg1); > + if (newphi != NULL) > { > + phi = newphi; > /* factor_out_conditional_conversion may create a new PHI in > BB2 and eliminate an existing PHI in BB2. Recompute values > that may be affected by that change. */ > - phis = phi_nodes (bb2); > - phi = single_non_singleton_phi_for_edges (phis, e1, e2); > - gcc_assert (phi); > arg0 = gimple_phi_arg_def (phi, e1->dest_idx); > arg1 = gimple_phi_arg_def (phi, e2->dest_idx); > - gcc_assert (arg0 != NULL && arg1 != NULL); > + gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE); > } > > /* Do the replacement of conditional if it can be done. */ > @@ -402,9 +402,9 @@ replace_phi_edge_with_variable (basic_block cond_block, > > /* PR66726: Factor conversion out of COND_EXPR. If the arguments of the PHI > stmt are CONVERT_STMT, factor out the conversion and perform the conversion > - to the result of PHI stmt. */ > + to the result of PHI stmt. Return the newly-created PHI, if any. */ > > -static bool > +static gphi * > factor_out_conditional_conversion (edge e0, edge e1, gphi *phi, > tree arg0, tree arg1) > { > @@ -421,7 +421,7 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi, > statement have the same unary operation, we can handle more > than two arguments too. */ > if (gimple_phi_num_args (phi) != 2) > - return false; > + return NULL; > > /* First canonicalize to simplify tests. */ > if (TREE_CODE (arg0) != SSA_NAME) > @@ -433,14 +433,14 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi, > if (TREE_CODE (arg0) != SSA_NAME > || (TREE_CODE (arg1) != SSA_NAME > && TREE_CODE (arg1) != INTEGER_CST)) > - return false; > + return NULL; > > /* Check if arg0 is an SSA_NAME and the stmt which defines arg0 is > a conversion. */ > arg0_def_stmt = SSA_NAME_DEF_STMT (arg0); > if (!is_gimple_assign (arg0_def_stmt) > || !gimple_assign_cast_p (arg0_def_stmt)) > - return false; > + return NULL; > > /* Use the RHS as new_arg0. */ > convert_code = gimple_assign_rhs_code (arg0_def_stmt); > @@ -455,7 +455,7 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi, > arg1_def_stmt = SSA_NAME_DEF_STMT (arg1); > if (!is_gimple_assign (arg1_def_stmt) > || gimple_assign_rhs_code (arg1_def_stmt) != convert_code) > - return false; > + return NULL; > > /* Use the RHS as new_arg1. */ > new_arg1 = gimple_assign_rhs1 (arg1_def_stmt); > @@ -471,21 +471,21 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi, > if (gimple_assign_cast_p (arg0_def_stmt)) > new_arg1 = fold_convert (TREE_TYPE (new_arg0), arg1); > else > - return false; > + return NULL; > } > else > - return false; > + return NULL; > } > > /* If arg0/arg1 have > 1 use, then this transformation actually increases > the number of expressions evaluated at runtime. */ > if (!has_single_use (arg0) > || (arg1_def_stmt && !has_single_use (arg1))) > - return false; > + return NULL; > > /* If types of new_arg0 and new_arg1 are different bailout. */ > if (!types_compatible_p (TREE_TYPE (new_arg0), TREE_TYPE (new_arg1))) > - return false; > + return NULL; > > /* Create a new PHI stmt. */ > result = PHI_RESULT (phi); > @@ -528,7 +528,7 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi, > /* Remove the original PHI stmt. */ > gsi = gsi_for_stmt (phi); > gsi_remove (&gsi, true); > - return true; > + return newphi; > } > > /* The function conditional_replacement does the main work of doing the > > Marek
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..d7e8aa0 100644 --- gcc/tree-ssa-phiopt.c +++ gcc/tree-ssa-phiopt.c @@ -49,7 +49,7 @@ along with GCC; see the file COPYING3. If not see static unsigned int tree_ssa_phiopt_worker (bool, bool); static bool conditional_replacement (basic_block, basic_block, edge, edge, gphi *, tree, tree); -static bool factor_out_conditional_conversion (edge, edge, gphi *, tree, tree); +static gphi *factor_out_conditional_conversion (edge, edge, gphi *, tree, tree); static int value_replacement (basic_block, basic_block, edge, edge, gimple *, tree, tree); static bool minmax_replacement (basic_block, basic_block, @@ -310,19 +310,19 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads) /* Something is wrong if we cannot find the arguments in the PHI node. */ - gcc_assert (arg0 != NULL && arg1 != NULL); + gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE); - if (factor_out_conditional_conversion (e1, e2, phi, arg0, arg1)) + gphi *newphi = factor_out_conditional_conversion (e1, e2, phi, + arg0, arg1); + if (newphi != NULL) { + phi = newphi; /* factor_out_conditional_conversion may create a new PHI in BB2 and eliminate an existing PHI in BB2. Recompute values that may be affected by that change. */ - phis = phi_nodes (bb2); - phi = single_non_singleton_phi_for_edges (phis, e1, e2); - gcc_assert (phi); arg0 = gimple_phi_arg_def (phi, e1->dest_idx); arg1 = gimple_phi_arg_def (phi, e2->dest_idx); - gcc_assert (arg0 != NULL && arg1 != NULL); + gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE); } /* Do the replacement of conditional if it can be done. */ @@ -402,9 +402,9 @@ replace_phi_edge_with_variable (basic_block cond_block, /* PR66726: Factor conversion out of COND_EXPR. If the arguments of the PHI stmt are CONVERT_STMT, factor out the conversion and perform the conversion - to the result of PHI stmt. */ + to the result of PHI stmt. Return the newly-created PHI, if any. */ -static bool +static gphi * factor_out_conditional_conversion (edge e0, edge e1, gphi *phi, tree arg0, tree arg1) { @@ -421,7 +421,7 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi, statement have the same unary operation, we can handle more than two arguments too. */ if (gimple_phi_num_args (phi) != 2) - return false; + return NULL; /* First canonicalize to simplify tests. */ if (TREE_CODE (arg0) != SSA_NAME) @@ -433,14 +433,14 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi, if (TREE_CODE (arg0) != SSA_NAME || (TREE_CODE (arg1) != SSA_NAME && TREE_CODE (arg1) != INTEGER_CST)) - return false; + return NULL; /* Check if arg0 is an SSA_NAME and the stmt which defines arg0 is a conversion. */ arg0_def_stmt = SSA_NAME_DEF_STMT (arg0); if (!is_gimple_assign (arg0_def_stmt) || !gimple_assign_cast_p (arg0_def_stmt)) - return false; + return NULL; /* Use the RHS as new_arg0. */ convert_code = gimple_assign_rhs_code (arg0_def_stmt); @@ -455,7 +455,7 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi, arg1_def_stmt = SSA_NAME_DEF_STMT (arg1); if (!is_gimple_assign (arg1_def_stmt) || gimple_assign_rhs_code (arg1_def_stmt) != convert_code) - return false; + return NULL; /* Use the RHS as new_arg1. */ new_arg1 = gimple_assign_rhs1 (arg1_def_stmt); @@ -471,21 +471,21 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi, if (gimple_assign_cast_p (arg0_def_stmt)) new_arg1 = fold_convert (TREE_TYPE (new_arg0), arg1); else - return false; + return NULL; } else - return false; + return NULL; } /* If arg0/arg1 have > 1 use, then this transformation actually increases the number of expressions evaluated at runtime. */ if (!has_single_use (arg0) || (arg1_def_stmt && !has_single_use (arg1))) - return false; + return NULL; /* If types of new_arg0 and new_arg1 are different bailout. */ if (!types_compatible_p (TREE_TYPE (new_arg0), TREE_TYPE (new_arg1))) - return false; + return NULL; /* Create a new PHI stmt. */ result = PHI_RESULT (phi); @@ -528,7 +528,7 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi, /* Remove the original PHI stmt. */ gsi = gsi_for_stmt (phi); gsi_remove (&gsi, true); - return true; + return newphi; } /* The function conditional_replacement does the main work of doing the