On Thu, 5 Jan 2017, Jakub Jelinek wrote:
> Hi!
>
> On Thu, Jan 05, 2017 at 09:09:06AM +0100, Richard Biener wrote:
> > I think that would be premature. Consider a modified
> > gcc.dg/tree-ssa/pr66726.c:
> >
> > extern unsigned short mode_size[];
> >
> > int
> > oof (int mode)
> > {
> > int tem;
> > if (64 < mode_size[mode])
> > tem = 64;
> > else
> > tem = mode_size[mode];
> > return tem;
> > }
>
> You're right, that testcase still needs factor_out_*.
>
> > That we now fold the GENERIC cond-expr during gimplification
> > is of course good but doesn't make the phiopt code useless.
> > As far as I remember I objected adding handling of conversions
> > in the minmax replacement code and instead suggested to add this
> > "enablement" phiopt instead. So what I suggest is to tame that down
> > to the cases where it actually enables something (empty BB afterwards,
> > a PHI with a use that's also used on the controlling conditional).
>
> So like this (if it passes bootstrap/regtest)?
Yes, this looks good to me.
Thanks,
Richard.
> 2017-01-05 Jakub Jelinek <ja...@redhat.com>
>
> PR tree-optimization/71016
> * tree-ssa-phiopt.c (tree_ssa_phiopt_worker): Pass cond_stmt to
> factor_out_conditional_conversion. Formatting fix.
> (factor_out_conditional_conversion): Add cond_stmt argument.
> If arg1 is INTEGER_CST, punt if new_arg0 is not any operand of
> cond_stmt and if arg0_def_stmt is not the only stmt in its bb.
>
> * gcc.target/i386/pr71016.c: New test.
> * gcc.target/aarch64/pr71016.c: New test.
> * gcc.dg/tree-ssa/pr66726-3.c: New test.
>
> --- gcc/tree-ssa-phiopt.c.jj 2017-01-03 08:12:27.000000000 +0100
> +++ gcc/tree-ssa-phiopt.c 2017-01-05 12:12:09.571017488 +0100
> @@ -49,7 +49,8 @@ along with GCC; see the file COPYING3.
> static unsigned int tree_ssa_phiopt_worker (bool, bool);
> static bool conditional_replacement (basic_block, basic_block,
> edge, edge, gphi *, tree, tree);
> -static gphi *factor_out_conditional_conversion (edge, edge, gphi *, tree,
> tree);
> +static gphi *factor_out_conditional_conversion (edge, edge, gphi *, tree,
> tree,
> + gimple *);
> static int value_replacement (basic_block, basic_block,
> edge, edge, gimple *, tree, tree);
> static bool minmax_replacement (basic_block, basic_block,
> @@ -233,7 +234,7 @@ tree_ssa_phiopt_worker (bool do_store_el
> continue;
> }
> else if (do_hoist_loads
> - && EDGE_SUCC (bb1, 0)->dest == EDGE_SUCC (bb2, 0)->dest)
> + && EDGE_SUCC (bb1, 0)->dest == EDGE_SUCC (bb2, 0)->dest)
> {
> basic_block bb3 = EDGE_SUCC (bb1, 0)->dest;
>
> @@ -313,7 +314,8 @@ tree_ssa_phiopt_worker (bool do_store_el
> gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
>
> gphi *newphi = factor_out_conditional_conversion (e1, e2, phi,
> - arg0, arg1);
> + arg0, arg1,
> + cond_stmt);
> if (newphi != NULL)
> {
> phi = newphi;
> @@ -402,11 +404,12 @@ replace_phi_edge_with_variable (basic_bl
>
> /* 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. Return the newly-created PHI, if any. */
> + to the result of PHI stmt. COND_STMT is the controlling predicate.
> + Return the newly-created PHI, if any. */
>
> static gphi *
> factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
> - tree arg0, tree arg1)
> + tree arg0, tree arg1, gimple *cond_stmt)
> {
> gimple *arg0_def_stmt = NULL, *arg1_def_stmt = NULL, *new_stmt;
> tree new_arg0 = NULL_TREE, new_arg1 = NULL_TREE;
> @@ -472,7 +475,31 @@ factor_out_conditional_conversion (edge
> && int_fits_type_p (arg1, TREE_TYPE (new_arg0)))
> {
> if (gimple_assign_cast_p (arg0_def_stmt))
> - new_arg1 = fold_convert (TREE_TYPE (new_arg0), arg1);
> + {
> + /* For the INTEGER_CST case, we are just moving the
> + conversion from one place to another, which can often
> + hurt as the conversion moves further away from the
> + statement that computes the value. So, perform this
> + only if new_arg0 is an operand of COND_STMT, or
> + if arg0_def_stmt is the only non-debug stmt in
> + its basic block, because then it is possible this
> + could enable further optimizations (minmax replacement
> + etc.). See PR71016. */
> + if (new_arg0 != gimple_cond_lhs (cond_stmt)
> + && new_arg0 != gimple_cond_rhs (cond_stmt)
> + && gimple_bb (arg0_def_stmt) == e0->src)
> + {
> + gsi = gsi_for_stmt (arg0_def_stmt);
> + gsi_prev_nondebug (&gsi);
> + if (!gsi_end_p (gsi))
> + return NULL;
> + gsi = gsi_for_stmt (arg0_def_stmt);
> + gsi_next_nondebug (&gsi);
> + if (!gsi_end_p (gsi))
> + return NULL;
> + }
> + new_arg1 = fold_convert (TREE_TYPE (new_arg0), arg1);
> + }
> else
> return NULL;
> }
> --- gcc/testsuite/gcc.target/i386/pr71016.c.jj 2017-01-05
> 12:06:33.808325995 +0100
> +++ gcc/testsuite/gcc.target/i386/pr71016.c 2017-01-05 12:07:54.242293867
> +0100
> @@ -0,0 +1,10 @@
> +/* PR tree-optimization/71016 */
> +/* { dg-do compile { target lp64 } } */
> +/* { dg-options "-O2 -mlzcnt" } */
> +/* { dg-final { scan-assembler-not "cltq" } } */
> +
> +long int
> +foo (long int i)
> +{
> + return i == 0 ? 17 : __builtin_clzl (i);
> +}
> --- gcc/testsuite/gcc.target/aarch64/pr71016.c.jj 2017-01-05
> 12:09:12.075295113 +0100
> +++ gcc/testsuite/gcc.target/aarch64/pr71016.c 2017-01-05
> 12:09:28.479084620 +0100
> @@ -0,0 +1,10 @@
> +/* PR tree-optimization/71016 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +/* { dg-final { scan-assembler-not "sxtw" } } */
> +
> +long int
> +foo (long int i)
> +{
> + return i == 0 ? 17 : __builtin_clzl (i);
> +}
> --- gcc/testsuite/gcc.dg/tree-ssa/pr66726-3.c.jj 2017-01-05
> 12:10:07.746580739 +0100
> +++ gcc/testsuite/gcc.dg/tree-ssa/pr66726-3.c 2017-01-05 11:41:59.000000000
> +0100
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> +/* { dg-final { scan-tree-dump-times "MIN_EXPR" 1 "optimized" } } */
> +
> +extern unsigned short mode_size[];
> +
> +int
> +oof (int mode)
> +{
> + int tem;
> + if (64 < mode_size[mode])
> + tem = 64;
> + else
> + tem = mode_size[mode];
> + return tem;
> +}
>
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
