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)?
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
