On Wed, Dec 9, 2015 at 3:22 PM, Marek Polacek <pola...@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 <pola...@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
