diff mbox

[RFC,PR61839] Convert CST BINOP COND_EXPR to COND_EXPR ? (CST BINOP 1) : (CST BINOP 0)

Message ID 5712C739.4080101@linaro.org
State New
Headers show

Commit Message

Kugan Vivekanandarajah April 16, 2016, 11:14 p.m. UTC
As explained in PR61839,

Following difference results in extra instructions:
-  c = b != 0 ? 486097858 : 972195717;
+  c = a + 972195718 >> (b != 0);

As suggested in PR, attached patch converts CST BINOP COND_EXPR to 
COND_EXPR ? (CST BINOP 1) : (CST BINOP 0).

Bootstrapped and regression tested for x86-64-linux-gnu with no new 
regression. Is this OK for statege-1.

Thanks,
Kugan

gcc/ChangeLog:

2016-04-17  Kugan Vivekanandarajah  <kuganv@linaro.org>

	* tree-vrp.c (simplify_stmt_using_ranges): Convert CST BINOP COND_EXPR to
	COND_EXPR ? (CST BINOP 1) : (CST BINOP 0) when possible.

Comments

Richard Biener April 29, 2016, 10:47 a.m. UTC | #1
On Sun, Apr 17, 2016 at 1:14 AM, kugan
<kugan.vivekanandarajah@linaro.org> wrote:
> As explained in PR61839,
>
> Following difference results in extra instructions:
> -  c = b != 0 ? 486097858 : 972195717;
> +  c = a + 972195718 >> (b != 0);
>
> As suggested in PR, attached patch converts CST BINOP COND_EXPR to COND_EXPR
> ? (CST BINOP 1) : (CST BINOP 0).
>
> Bootstrapped and regression tested for x86-64-linux-gnu with no new
> regression. Is this OK for statege-1.

You are missing a testcase.

I think the transform can be generalized to any two-value value-range by
instead of

  lhs = cond_res ? (cst binop 1) : (cst binop 0)

emitting

  lhs = tmp == val1 ? (cst binop val1) : (cst binop val2);

In the PR I asked the transform to be only carried out if cond_res and
tmp have a single use (and thus they'd eventually vanish).

I'm not sure if a general two-value "constant" propagation is profitable
which is why I was originally asking for the pattern to only apply
if the resulting value is used in a comparison which we could then
in turn simplify by substituting COND_RES (or ! COND_RES) for it.
For the general two-value case we'd substitute it with tmp [=!]= val[12]
dependent on which constant is cheaper to test for.

So I think this needs some exploring work on which way to go
and which transform is profitable in the end.  I think the general
two-value case feeding a condition will be always profitable.

Thanks,
Richard.

> Thanks,
> Kugan
>
> gcc/ChangeLog:
>
> 2016-04-17  Kugan Vivekanandarajah  <kuganv@linaro.org>
>

Add      PR tree-optimization/61839

>         * tree-vrp.c (simplify_stmt_using_ranges): Convert CST BINOP
> COND_EXPR to
>         COND_EXPR ? (CST BINOP 1) : (CST BINOP 0) when possible.
diff mbox

Patch

diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index bbdf9ce..caf7a2a 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -9902,6 +9902,49 @@  simplify_stmt_using_ranges (gimple_stmt_iterator *gsi)
     {
       enum tree_code rhs_code = gimple_assign_rhs_code (stmt);
       tree rhs1 = gimple_assign_rhs1 (stmt);
+      tree rhs2 = gimple_assign_rhs2 (stmt);
+      tree var;
+
+      /* Convert:
+	 COND_RES = X COMPARE Y
+	 TMP = (CAST) COND_RES
+	 LHS = CST BINOP TMP
+
+	 To:
+	 LHS = COND_RES ? (CST BINOP 1) : (CST BINOP 0) */
+
+      if (TREE_CODE_CLASS (rhs_code) == tcc_binary
+	  && TREE_CODE (rhs1) == INTEGER_CST
+	  && TREE_CODE (rhs2) == SSA_NAME
+	  && is_gimple_assign (SSA_NAME_DEF_STMT (rhs2))
+	  && gimple_assign_rhs_code (SSA_NAME_DEF_STMT (rhs2)) == NOP_EXPR
+	  && (var = gimple_assign_rhs1 (SSA_NAME_DEF_STMT (rhs2)))
+	  && TREE_CODE (var) == SSA_NAME
+	  && is_gimple_assign (SSA_NAME_DEF_STMT (var))
+	  && TREE_CODE_CLASS (gimple_assign_rhs_code (SSA_NAME_DEF_STMT (var)))
+	  == tcc_comparison)
+
+	{
+	  value_range *vr = get_value_range (var);
+	  if (range_int_cst_p (vr)
+	      && integer_zerop (vr->min)
+	      && integer_onep (vr->max))
+	    {
+
+	      tree new_rhs1 =  int_const_binop (rhs_code, rhs1, vr->max);
+	      tree new_rhs2 =  int_const_binop (rhs_code, rhs1, vr->min);
+
+	      if (new_rhs1 && new_rhs2)
+		{
+		  gimple_assign_set_rhs_with_ops (gsi,
+						  COND_EXPR, var,
+						  new_rhs1,
+						  new_rhs2);
+		  update_stmt (gsi_stmt (*gsi));
+		  return true;
+		}
+	    }
+	}
 
       switch (rhs_code)
 	{