diff mbox

[RFC,PR40921] Convert x + (-y * z * z) into x - y * z * z

Message ID 572A96AE.10701@linaro.org
State New
Headers show

Commit Message

Kugan Vivekanandarajah May 5, 2016, 12:41 a.m. UTC 
Hi Richard,

>
> +             int last = ops.length () - 1;
> +             bool negate_result = false;
>
> Do
>
>    oe &last = ops.last ();
>

Done.

>
> +             if (rhs_code == MULT_EXPR
> +                 && ops.length () > 1
> +                 && ((TREE_CODE (ops[last]->op) == INTEGER_CST
>
> and last.op here and below
>
> +                      && integer_minus_onep (ops[last]->op))
> +                     || ((TREE_CODE (ops[last]->op) == REAL_CST)
> +                         && real_equal (&TREE_REAL_CST
> (ops[last]->op), &dconstm1))))
>

Done.

> Here the checks !HONOR_SNANS () && (!HONOS_SIGNED_ZEROS ||
> !COMPLEX_FLOAT_TYPE_P)
> are missing.  The * -1 might appear literally and you are only allowed
> to turn it into a negate
> under the above conditions.

Done.

>
> +               {
> +                 ops.unordered_remove (last);
>
> use ops.pop ();
>
Done.

> +                 negate_result = true;
>
> Please move the whole thing under the else { } case of the ops.length
> == 0, ops.length == 1 test chain
> as you did for the actual emit of the negate.
>

I see your point. However, when we remove the (-1) from the ops list, 
that intern can result in ops.length becoming 1. Therefore, I moved the 
  the following  if (negate_result), outside the condition.


>
> +                 if (negate_result)
> +                   {
> +                     tree tmp = make_ssa_name (TREE_TYPE (lhs));
> +                     gimple_set_lhs (stmt, tmp);
> +                     gassign *neg_stmt = gimple_build_assign (lhs, NEGATE_EXPR,
> +                                                              tmp);
> +                     gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
> +                     gsi_insert_after (&gsi, neg_stmt, GSI_NEW_STMT);
> +                     update_stmt (stmt);
>
> I think that if powi_result is also built you end up using the wrong
> stmt so you miss a
>
>                          stmt = SSA_NAME_DEF_STMT (lhs);

Yes, indeed. This can happen and I have added this.

>
> here.  Also see the new_lhs handling of the powi_result case - again
> you need sth
> similar here (it's handling looks a bit fishy as well - this all needs
> some comments
> and possibly a (lot more) testcases).
>
> So, please do the above requested changes and verify the 'lhs' issues I pointed
> out by trying to add a few more testcase that also cover the case where a powi
> is detected in addition to a negation.  Please also add a testcase that catches
> (-y) * x * (-z).
>

Added this to the testcase.

Does this look better now?

Thanks,
Kugan


>> 2016-04-23  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>
>>          PR middle-end/40921
>>          * gcc.dg/tree-ssa/pr40921.c: New test.
>>
>> gcc/ChangeLog:
>>
>> 2016-04-23  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>
>>          PR middle-end/40921
>>          * tree-ssa-reassoc.c (try_special_add_to_ops): New.
>>          (linearize_expr_tree): Call try_special_add_to_ops.
>>          (reassociate_bb): Convert MULT_EXPR by (-1) to NEGATE_EXPR.
>>

Comments

Richard Biener May 9, 2016, 11:10 a.m. UTC | #1 
On Thu, May 5, 2016 at 2:41 AM, kugan <kugan.vivekanandarajah@linaro.org> wrote:
> Hi Richard,
>
>>
>> +             int last = ops.length () - 1;
>> +             bool negate_result = false;
>>
>> Do
>>
>>    oe &last = ops.last ();
>>
>
> Done.
>
>>
>> +             if (rhs_code == MULT_EXPR
>> +                 && ops.length () > 1
>> +                 && ((TREE_CODE (ops[last]->op) == INTEGER_CST
>>
>> and last.op here and below
>>
>> +                      && integer_minus_onep (ops[last]->op))
>> +                     || ((TREE_CODE (ops[last]->op) == REAL_CST)
>> +                         && real_equal (&TREE_REAL_CST
>> (ops[last]->op), &dconstm1))))
>>
>
> Done.
>
>> Here the checks !HONOR_SNANS () && (!HONOS_SIGNED_ZEROS ||
>> !COMPLEX_FLOAT_TYPE_P)
>> are missing.  The * -1 might appear literally and you are only allowed
>> to turn it into a negate
>> under the above conditions.
>
>
> Done.
>
>>
>> +               {
>> +                 ops.unordered_remove (last);
>>
>> use ops.pop ();
>>
> Done.
>
>> +                 negate_result = true;
>>
>> Please move the whole thing under the else { } case of the ops.length
>> == 0, ops.length == 1 test chain
>> as you did for the actual emit of the negate.
>>
>
> I see your point. However, when we remove the (-1) from the ops list, that
> intern can result in ops.length becoming 1. Therefore, I moved the  the
> following  if (negate_result), outside the condition.

Ah, indeed.   But now you have to care for ops.length () == 0 and thus
the unconditonally ops.last () may now trap.  So I suggest to
do

+             operand_entry *last;
+             bool negate_result = false;
+             if (rhs_code == MULT_EXPR
+                 && ops.length () > 1
                   && (last = ops.last ())
+                 && ((TREE_CODE (last->op) == INTEGER_CST

to avoid this.

>
>>
>> +                 if (negate_result)
>> +                   {
>> +                     tree tmp = make_ssa_name (TREE_TYPE (lhs));
>> +                     gimple_set_lhs (stmt, tmp);
>> +                     gassign *neg_stmt = gimple_build_assign (lhs,
>> NEGATE_EXPR,
>> +                                                              tmp);
>> +                     gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
>> +                     gsi_insert_after (&gsi, neg_stmt, GSI_NEW_STMT);
>> +                     update_stmt (stmt);
>>
>> I think that if powi_result is also built you end up using the wrong
>> stmt so you miss a
>>
>>                          stmt = SSA_NAME_DEF_STMT (lhs);
>
>
> Yes, indeed. This can happen and I have added this.
>
>>
>> here.  Also see the new_lhs handling of the powi_result case - again
>> you need sth
>> similar here (it's handling looks a bit fishy as well - this all needs
>> some comments
>> and possibly a (lot more) testcases).
>>
>> So, please do the above requested changes and verify the 'lhs' issues I
>> pointed
>> out by trying to add a few more testcase that also cover the case where a
>> powi
>> is detected in addition to a negation.  Please also add a testcase that
>> catches
>> (-y) * x * (-z).
>>
>
> Added this to the testcase.
>
> Does this look better now?

Yes - the patch is ok with the above suggested change.

Thanks,
Richard.

>
> Thanks,
> Kugan
>
>
>>> 2016-04-23  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>>
>>>          PR middle-end/40921
>>>          * gcc.dg/tree-ssa/pr40921.c: New test.
>>>
>>> gcc/ChangeLog:
>>>
>>> 2016-04-23  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>>
>>>          PR middle-end/40921
>>>          * tree-ssa-reassoc.c (try_special_add_to_ops): New.
>>>          (linearize_expr_tree): Call try_special_add_to_ops.
>>>          (reassociate_bb): Convert MULT_EXPR by (-1) to NEGATE_EXPR.
>>>
>
diff mbox

Patch

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr40921.c b/gcc/testsuite/gcc.dg/tree-ssa/pr40921.c
index e69de29..3a5a23a 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr40921.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr40921.c
@@ -0,0 +1,26 @@ 
+
+/* { dg-do compile } */
+/* { dg-options "-O2  -fdump-tree-optimized -ffast-math" } */
+
+unsigned int foo (unsigned int x, unsigned int y, unsigned int z)
+{
+      return x + (-y * z * z);
+}
+
+float bar (float x, float y, float z)
+{
+      return x + (-y * z * z);
+}
+
+float bar2 (float x, float y, float z)
+{
+      return x + (-y * z * z * 5.0f);
+}
+
+float bar3 (float x, float y, float z)
+{
+      return x + (-y * x * -z);
+}
+
+
+/* { dg-final { scan-tree-dump-times "_* = -y_" 0 "optimized" } } */
diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
index 4e1251b..1df6681 100644
--- a/gcc/tree-ssa-reassoc.c
+++ b/gcc/tree-ssa-reassoc.c
@@ -4112,6 +4112,45 @@  acceptable_pow_call (gimple *stmt, tree *base, HOST_WIDE_INT *exponent)
   return true;
 }
 
+/* Try to derive and add operand entry for OP to *OPS.  Return false if
+   unsuccessful.  */
+
+static bool
+try_special_add_to_ops (vec<operand_entry *> *ops,
+			enum tree_code code,
+			tree op, gimple* def_stmt)
+{
+  tree base = NULL_TREE;
+  HOST_WIDE_INT exponent = 0;
+
+  if (TREE_CODE (op) != SSA_NAME)
+    return false;
+
+  if (code == MULT_EXPR
+      && acceptable_pow_call (def_stmt, &base, &exponent))
+    {
+      add_repeat_to_ops_vec (ops, base, exponent);
+      gimple_set_visited (def_stmt, true);
+      return true;
+    }
+  else if (code == MULT_EXPR
+	   && is_gimple_assign (def_stmt)
+	   && gimple_assign_rhs_code (def_stmt) == NEGATE_EXPR
+	   && !HONOR_SNANS (TREE_TYPE (op))
+	   && (!HONOR_SIGNED_ZEROS (TREE_TYPE (op))
+	       || !COMPLEX_FLOAT_TYPE_P (TREE_TYPE (op))))
+    {
+      tree rhs1 = gimple_assign_rhs1 (def_stmt);
+      tree cst = build_minus_one_cst (TREE_TYPE (op));
+      add_to_ops_vec (ops, rhs1);
+      add_to_ops_vec (ops, cst);
+      gimple_set_visited (def_stmt, true);
+      return true;
+    }
+
+  return false;
+}
+
 /* Recursively linearize a binary expression that is the RHS of STMT.
    Place the operands of the expression tree in the vector named OPS.  */
 
@@ -4126,8 +4165,6 @@  linearize_expr_tree (vec<operand_entry *> *ops, gimple *stmt,
   bool binrhsisreassoc = false;
   enum tree_code rhscode = gimple_assign_rhs_code (stmt);
   struct loop *loop = loop_containing_stmt (stmt);
-  tree base = NULL_TREE;
-  HOST_WIDE_INT exponent = 0;
 
   if (set_visited)
     gimple_set_visited (stmt, true);
@@ -4163,24 +4200,10 @@  linearize_expr_tree (vec<operand_entry *> *ops, gimple *stmt,
 
       if (!binrhsisreassoc)
 	{
-	  if (rhscode == MULT_EXPR
-	      && TREE_CODE (binrhs) == SSA_NAME
-	      && acceptable_pow_call (binrhsdef, &base, &exponent))
-	    {
-	      add_repeat_to_ops_vec (ops, base, exponent);
-	      gimple_set_visited (binrhsdef, true);
-	    }
-	  else
+	  if (!try_special_add_to_ops (ops, rhscode, binrhs, binrhsdef))
 	    add_to_ops_vec (ops, binrhs);
 
-	  if (rhscode == MULT_EXPR
-	      && TREE_CODE (binlhs) == SSA_NAME
-	      && acceptable_pow_call (binlhsdef, &base, &exponent))
-	    {
-	      add_repeat_to_ops_vec (ops, base, exponent);
-	      gimple_set_visited (binlhsdef, true);
-	    }
-	  else
+	  if (!try_special_add_to_ops (ops, rhscode, binlhs, binlhsdef))
 	    add_to_ops_vec (ops, binlhs);
 
 	  return;
@@ -4220,14 +4243,7 @@  linearize_expr_tree (vec<operand_entry *> *ops, gimple *stmt,
   linearize_expr_tree (ops, SSA_NAME_DEF_STMT (binlhs),
 		       is_associative, set_visited);
 
-  if (rhscode == MULT_EXPR
-      && TREE_CODE (binrhs) == SSA_NAME
-      && acceptable_pow_call (SSA_NAME_DEF_STMT (binrhs), &base, &exponent))
-    {
-      add_repeat_to_ops_vec (ops, base, exponent);
-      gimple_set_visited (SSA_NAME_DEF_STMT (binrhs), true);
-    }
-  else
+  if (!try_special_add_to_ops (ops, rhscode, binrhs, binrhsdef))
     add_to_ops_vec (ops, binrhs);
 }
 
@@ -4980,6 +4996,22 @@  reassociate_bb (basic_block bb)
 		  && flag_unsafe_math_optimizations)
 		powi_result = attempt_builtin_powi (stmt, &ops);
 
+	      operand_entry *last = ops.last ();
+	      bool negate_result = false;
+	      if (rhs_code == MULT_EXPR
+		  && ops.length () > 1
+		  && ((TREE_CODE (last->op) == INTEGER_CST
+		       && integer_minus_onep (last->op))
+		      || ((TREE_CODE (last->op) == REAL_CST)
+			  && real_equal (&TREE_REAL_CST (last->op), &dconstm1)))
+		  && !HONOR_SNANS (TREE_TYPE (lhs))
+		  && (!HONOR_SIGNED_ZEROS (TREE_TYPE (lhs))
+		      || !COMPLEX_FLOAT_TYPE_P (TREE_TYPE (lhs))))
+		{
+		  ops.pop ();
+		  negate_result = true;
+		}
+
 	      /* If the operand vector is now empty, all operands were 
 		 consumed by the __builtin_powi optimization.  */
 	      if (ops.length () == 0)
@@ -5042,6 +5074,18 @@  reassociate_bb (basic_block bb)
 		      gsi_insert_after (&gsi, mul_stmt, GSI_NEW_STMT);
 		    }
 		}
+
+	      if (negate_result)
+		{
+		  stmt = SSA_NAME_DEF_STMT (lhs);
+		  tree tmp = make_ssa_name (TREE_TYPE (lhs));
+		  gimple_set_lhs (stmt, tmp);
+		  gassign *neg_stmt = gimple_build_assign (lhs, NEGATE_EXPR,
+							   tmp);
+		  gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
+		  gsi_insert_after (&gsi, neg_stmt, GSI_NEW_STMT);
+		  update_stmt (stmt);
+		}
 	    }
 	}
     }