[PR72835] Incorrect arithmetic optimization involving bitfield arguments

[PR72835] Incorrect arithmetic optimization involving bitfield arguments

[kugan]

[-- Attachment #1: Type: text/plain, Size: 1494 bytes --]

Hi,

The test-case in PR72835 is failing with -O2 and passing with 
-fno-tree-reassoc. It also passes with -O2 -fno-tree-vrp.

diff of .115t.dse2 and .116t.reassoc1 for the c++ testcase is as 
follows, which looks OK.

+  unsigned int _16;
+  unsigned int _17;
+  unsigned int _18;

    <bb 2>:
    _1 = s1.m2;
    _2 = (unsigned int) _1;
    _3 = s1.m3;
    _4 = (unsigned int) _3;
-  _5 = -_4;
-  _6 = _2 * _5;
+  _5 = _4;
+  _6 = _5 * _2;
    var_32.0_7 = var_32;
    _8 = (unsigned int) var_32.0_7;
    _9 = s1.m1;
    _10 = (unsigned int) _9;
-  _11 = -_10;
-  _12 = _8 * _11;
-  c_14 = _6 + _12;
+  _11 = _10;
+  _12 = _11 * _8;
+  _16 = _12 + _6;
+  _18 = _16;
+  _17 = -_18;
+  c_14 = _17;
    if (c_14 != 4098873984)


However, I noticed that when we re-associate and assign different 
operands to the stmts, we are not resetting the flow information to the 
LHS. This looks wrong. Attached patch resets it. With this, the 
testcases in TH PR is now working.


Bootstrap and regression testing for x86_64-linux-gnu is in progress. Is 
this OK for trunk if there is no regression.

Thanks,
Kugan

gcc/testsuite/ChangeLog:

2016-08-09  Kugan Vivekanandarajah  <kuganv@linaro.org>

	PR tree-optimization/72835
	* gcc.dg/torture/pr72835.c: New test.

gcc/ChangeLog:

2016-08-09  Kugan Vivekanandarajah  <kuganv@linaro.org>

	PR tree-optimization/72835
	* tree-ssa-reassoc.c (rewrite_expr_tree): Reset value_range of LHS when
	operands are changed.
	(rewrite_expr_tree_parallel): Likewise.

[-- Attachment #2: pr72835.txt --]
[-- Type: text/plain, Size: 1609 bytes --]

diff --git a/gcc/testsuite/gcc.dg/torture/pr72835.c b/gcc/testsuite/gcc.dg/torture/pr72835.c
index e69de29..d7d0a8d 100644
--- a/gcc/testsuite/gcc.dg/torture/pr72835.c
+++ b/gcc/testsuite/gcc.dg/torture/pr72835.c
@@ -0,0 +1,36 @@
+/* PR tree-optimization/72835 */
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+struct struct_1 {
+    unsigned int m1 : 6 ;
+    unsigned int m2 : 24 ;
+    unsigned int m3 : 6 ;
+};
+
+unsigned short var_32 = 0x2d10;
+
+struct struct_1 s1;
+
+void init ()
+{
+  s1.m1 = 4;
+  s1.m2 = 0x7ca4b8;
+  s1.m3 = 24;
+}
+
+void foo ()
+{
+  unsigned int c =
+    ((unsigned int) s1.m2) * (-((unsigned int) s1.m3))
+    + (var_32) * (-((unsigned int) (s1.m1)));
+  if (c != 4098873984)
+    __builtin_abort ();
+}
+
+int main ()
+{
+    init ();
+    foo ();
+    return 0;
+}
diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
index 7fd7550..b864ed1 100644
--- a/gcc/tree-ssa-reassoc.c
+++ b/gcc/tree-ssa-reassoc.c
@@ -3945,6 +3945,7 @@ rewrite_expr_tree (gimple *stmt, unsigned int opindex,
 	      gimple_assign_set_rhs1 (stmt, oe1->op);
 	      gimple_assign_set_rhs2 (stmt, oe2->op);
 	      update_stmt (stmt);
+	      reset_flow_sensitive_info (lhs);
 	    }
 
 	  if (rhs1 != oe1->op && rhs1 != oe2->op)
@@ -4193,9 +4194,11 @@ rewrite_expr_tree_parallel (gassign *stmt, int width,
 	 others are recreated.  */
       if (i == stmt_num - 1)
 	{
+	  tree lhs = gimple_assign_lhs (stmts[i]);
 	  gimple_assign_set_rhs1 (stmts[i], op1);
 	  gimple_assign_set_rhs2 (stmts[i], op2);
 	  update_stmt (stmts[i]);
+	  reset_flow_sensitive_info (lhs);
 	}
       else
 	{

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[kugan]

[-- Attachment #1: Type: text/plain, Size: 1759 bytes --]



On 09/08/16 23:43, kugan wrote:
> Hi,
>
> The test-case in PR72835 is failing with -O2 and passing with
> -fno-tree-reassoc. It also passes with -O2 -fno-tree-vrp.
>
> diff of .115t.dse2 and .116t.reassoc1 for the c++ testcase is as
> follows, which looks OK.
>
> +  unsigned int _16;
> +  unsigned int _17;
> +  unsigned int _18;
>
>     <bb 2>:
>     _1 = s1.m2;
>     _2 = (unsigned int) _1;
>     _3 = s1.m3;
>     _4 = (unsigned int) _3;
> -  _5 = -_4;
> -  _6 = _2 * _5;
> +  _5 = _4;
> +  _6 = _5 * _2;
>     var_32.0_7 = var_32;
>     _8 = (unsigned int) var_32.0_7;
>     _9 = s1.m1;
>     _10 = (unsigned int) _9;
> -  _11 = -_10;
> -  _12 = _8 * _11;
> -  c_14 = _6 + _12;
> +  _11 = _10;
> +  _12 = _11 * _8;
> +  _16 = _12 + _6;
> +  _18 = _16;
> +  _17 = -_18;
> +  c_14 = _17;
>     if (c_14 != 4098873984)
>
>
> However, I noticed that when we re-associate and assign different
> operands to the stmts, we are not resetting the flow information to the
> LHS. This looks wrong. Attached patch resets it. With this, the
> testcases in TH PR is now working.
>
>
> Bootstrap and regression testing for x86_64-linux-gnu is in progress. Is
> this OK for trunk if there is no regression.

There was no new regression while testing. I also moved the testcase 
from gcc.dg/torture/pr72835.c to gcc.dg/tree-ssa/pr72835.c. Is this OK 
for trunk?

Thanks,
Kugan

gcc/testsuite/ChangeLog:

2016-08-10  Kugan Vivekanandarajah  <kuganv@linaro.org>

	PR tree-optimization/72835
	* gcc.dg/tree-ssa/pr72835.c: New test.

gcc/ChangeLog:

2016-08-10  Kugan Vivekanandarajah  <kuganv@linaro.org>

	PR tree-optimization/72835
	* tree-ssa-reassoc.c (rewrite_expr_tree): Reset value_range of LHS when
	operands are changed.
	(rewrite_expr_tree_parallel): Likewise.

[-- Attachment #2: pr72835.txt --]
[-- Type: text/plain, Size: 1613 bytes --]

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c b/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
index e69de29..d7d0a8d 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
@@ -0,0 +1,36 @@
+/* PR tree-optimization/72835 */
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+struct struct_1 {
+    unsigned int m1 : 6 ;
+    unsigned int m2 : 24 ;
+    unsigned int m3 : 6 ;
+};
+
+unsigned short var_32 = 0x2d10;
+
+struct struct_1 s1;
+
+void init ()
+{
+  s1.m1 = 4;
+  s1.m2 = 0x7ca4b8;
+  s1.m3 = 24;
+}
+
+void foo ()
+{
+  unsigned int c =
+    ((unsigned int) s1.m2) * (-((unsigned int) s1.m3))
+    + (var_32) * (-((unsigned int) (s1.m1)));
+  if (c != 4098873984)
+    __builtin_abort ();
+}
+
+int main ()
+{
+    init ();
+    foo ();
+    return 0;
+}
diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
index 7fd7550..b864ed1 100644
--- a/gcc/tree-ssa-reassoc.c
+++ b/gcc/tree-ssa-reassoc.c
@@ -3945,6 +3945,7 @@ rewrite_expr_tree (gimple *stmt, unsigned int opindex,
 	      gimple_assign_set_rhs1 (stmt, oe1->op);
 	      gimple_assign_set_rhs2 (stmt, oe2->op);
 	      update_stmt (stmt);
+	      reset_flow_sensitive_info (lhs);
 	    }
 
 	  if (rhs1 != oe1->op && rhs1 != oe2->op)
@@ -4193,9 +4194,11 @@ rewrite_expr_tree_parallel (gassign *stmt, int width,
 	 others are recreated.  */
       if (i == stmt_num - 1)
 	{
+	  tree lhs = gimple_assign_lhs (stmts[i]);
 	  gimple_assign_set_rhs1 (stmts[i], op1);
 	  gimple_assign_set_rhs2 (stmts[i], op2);
 	  update_stmt (stmts[i]);
+	  reset_flow_sensitive_info (lhs);
 	}
       else
 	{

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[Jakub Jelinek]

On Wed, Aug 10, 2016 at 07:42:25AM +1000, kugan wrote:
> There was no new regression while testing. I also moved the testcase from
> gcc.dg/torture/pr72835.c to gcc.dg/tree-ssa/pr72835.c. Is this OK for trunk?

This looks strange.  The tree-ssa-reassoc.c code has been trying to never
reuse SSA_NAMEs if they would hold a different value.
So there should be no resetting of flow sensitive info needed.

> gcc/testsuite/ChangeLog:
> 
> 2016-08-10  Kugan Vivekanandarajah  <kuganv@linaro.org>
> 
> 	PR tree-optimization/72835
> 	* gcc.dg/tree-ssa/pr72835.c: New test.
> 
> gcc/ChangeLog:
> 
> 2016-08-10  Kugan Vivekanandarajah  <kuganv@linaro.org>
> 
> 	PR tree-optimization/72835
> 	* tree-ssa-reassoc.c (rewrite_expr_tree): Reset value_range of LHS when
> 	operands are changed.
> 	(rewrite_expr_tree_parallel): Likewise.

	Jakub

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[kugan]

Hi Jakub,

On 10/08/16 07:46, Jakub Jelinek wrote:
> On Wed, Aug 10, 2016 at 07:42:25AM +1000, kugan wrote:
>> There was no new regression while testing. I also moved the testcase from
>> gcc.dg/torture/pr72835.c to gcc.dg/tree-ssa/pr72835.c. Is this OK for trunk?
>
> This looks strange.  The tree-ssa-reassoc.c code has been trying to never
> reuse SSA_NAMEs if they would hold a different value.
> So there should be no resetting of flow sensitive info needed.

We are not reusing but, if you see the example change in reassoc:

-  _5 = -_4;
-  _6 = _2 * _5;
+  _5 = _4;
+  _6 = _5 * _2;

_5 and _6 will now have different value ranges because they compute 
different values. Therefore I think we should reset (?).

Thanks,
Kugan


>
>> gcc/testsuite/ChangeLog:
>>
>> 2016-08-10  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>
>> 	PR tree-optimization/72835
>> 	* gcc.dg/tree-ssa/pr72835.c: New test.
>>
>> gcc/ChangeLog:
>>
>> 2016-08-10  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>
>> 	PR tree-optimization/72835
>> 	* tree-ssa-reassoc.c (rewrite_expr_tree): Reset value_range of LHS when
>> 	operands are changed.
>> 	(rewrite_expr_tree_parallel): Likewise.
>
> 	Jakub
>

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[Jakub Jelinek]

On Wed, Aug 10, 2016 at 07:51:08AM +1000, kugan wrote:
> On 10/08/16 07:46, Jakub Jelinek wrote:
> >On Wed, Aug 10, 2016 at 07:42:25AM +1000, kugan wrote:
> >>There was no new regression while testing. I also moved the testcase from
> >>gcc.dg/torture/pr72835.c to gcc.dg/tree-ssa/pr72835.c. Is this OK for trunk?
> >
> >This looks strange.  The tree-ssa-reassoc.c code has been trying to never
> >reuse SSA_NAMEs if they would hold a different value.
> >So there should be no resetting of flow sensitive info needed.
> 
> We are not reusing but, if you see the example change in reassoc:
> 
> -  _5 = -_4;
> -  _6 = _2 * _5;
> +  _5 = _4;
> +  _6 = _5 * _2;
> 
> _5 and _6 will now have different value ranges because they compute
> different values. Therefore I think we should reset (?).

No.  We should not have reused _5 and _6 for the different values.
It is not harmful just for the value ranges, but also for debug info.

	Jakub

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[kugan]

[-- Attachment #1: Type: text/plain, Size: 2016 bytes --]

Hi Jakub,

On 10/08/16 07:55, Jakub Jelinek wrote:
> On Wed, Aug 10, 2016 at 07:51:08AM +1000, kugan wrote:
>> On 10/08/16 07:46, Jakub Jelinek wrote:
>>> On Wed, Aug 10, 2016 at 07:42:25AM +1000, kugan wrote:
>>>> There was no new regression while testing. I also moved the testcase from
>>>> gcc.dg/torture/pr72835.c to gcc.dg/tree-ssa/pr72835.c. Is this OK for trunk?
>>>
>>> This looks strange.  The tree-ssa-reassoc.c code has been trying to never
>>> reuse SSA_NAMEs if they would hold a different value.
>>> So there should be no resetting of flow sensitive info needed.
>>
>> We are not reusing but, if you see the example change in reassoc:
>>
>> -  _5 = -_4;
>> -  _6 = _2 * _5;
>> +  _5 = _4;
>> +  _6 = _5 * _2;
>>
>> _5 and _6 will now have different value ranges because they compute
>> different values. Therefore I think we should reset (?).
>
> No.  We should not have reused _5 and _6 for the different values.
> It is not harmful just for the value ranges, but also for debug info.

I see it now. The problem is we are just looking at (-1) being in the 
ops list for passing changed to rewrite_expr_tree in the case of 
multiplication by negate.  If we have combined (-1), as in the testcase, 
we will not have the (-1) and will pass changed=false to rewrite_expr_tree.

We should set changed based on what happens in try_special_add_to_ops. 
Attached patch does this. Bootstrap and regression testing are ongoing. 
Is this OK for trunk if there is no regression.

Thanks,
Kugan


gcc/testsuite/ChangeLog:

2016-08-10  Kugan Vivekanandarajah  <kuganv@linaro.org>

	PR tree-optimization/72835
	* gcc.dg/tree-ssa/pr72835.c: New test.

gcc/ChangeLog:

2016-08-10  Kugan Vivekanandarajah  <kuganv@linaro.org>

	PR tree-optimization/72835
	* tree-ssa-reassoc.c (try_special_add_to_ops): Return true if we 
changed the operands.
	(linearize_expr_tree): Return true if try_special_add_top_ops set 
changed to true.
	(reassociate_bb): Pass changed returned by linearlize_expr_tree to 
rewrite_expr_tree.


[-- Attachment #2: pr72835_2.txt --]
[-- Type: text/plain, Size: 5298 bytes --]

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c b/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
index e69de29..d7d0a8d 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
@@ -0,0 +1,36 @@
+/* PR tree-optimization/72835 */
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+struct struct_1 {
+    unsigned int m1 : 6 ;
+    unsigned int m2 : 24 ;
+    unsigned int m3 : 6 ;
+};
+
+unsigned short var_32 = 0x2d10;
+
+struct struct_1 s1;
+
+void init ()
+{
+  s1.m1 = 4;
+  s1.m2 = 0x7ca4b8;
+  s1.m3 = 24;
+}
+
+void foo ()
+{
+  unsigned int c =
+    ((unsigned int) s1.m2) * (-((unsigned int) s1.m3))
+    + (var_32) * (-((unsigned int) (s1.m1)));
+  if (c != 4098873984)
+    __builtin_abort ();
+}
+
+int main ()
+{
+    init ();
+    foo ();
+    return 0;
+}
diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
index 7fd7550..c5641fe 100644
--- a/gcc/tree-ssa-reassoc.c
+++ b/gcc/tree-ssa-reassoc.c
@@ -1038,7 +1038,7 @@ eliminate_using_constants (enum tree_code opcode,
 }
 
 
-static void linearize_expr_tree (vec<operand_entry *> *, gimple *,
+static bool linearize_expr_tree (vec<operand_entry *> *, gimple *,
 				 bool, bool);
 
 /* Structure for tracking and counting operands.  */
@@ -4456,12 +4456,16 @@ acceptable_pow_call (gcall *stmt, tree *base, HOST_WIDE_INT *exponent)
 }
 
 /* Try to derive and add operand entry for OP to *OPS.  Return false if
-   unsuccessful.  */
+   unsuccessful.  If we changed the operands such that the (intermediate)
+   results can be different (as in the case of NEGATE_EXPR converted to
+   multiplication by -1), set changed to true so that we will not reuse the
+   SSA (PR72835).  */
 
 static bool
 try_special_add_to_ops (vec<operand_entry *> *ops,
 			enum tree_code code,
-			tree op, gimple* def_stmt)
+			tree op, gimple* def_stmt,
+			bool *changed)
 {
   tree base = NULL_TREE;
   HOST_WIDE_INT exponent = 0;
@@ -4492,6 +4496,7 @@ try_special_add_to_ops (vec<operand_entry *> *ops,
       add_to_ops_vec (ops, rhs1);
       add_to_ops_vec (ops, cst);
       gimple_set_visited (def_stmt, true);
+      *changed = true;
       return true;
     }
 
@@ -4499,9 +4504,10 @@ try_special_add_to_ops (vec<operand_entry *> *ops,
 }
 
 /* Recursively linearize a binary expression that is the RHS of STMT.
-   Place the operands of the expression tree in the vector named OPS.  */
+   Place the operands of the expression tree in the vector named OPS.
+   Return TRUE if try_special_add_to_ops has set changed to TRUE.  */
 
-static void
+static bool
 linearize_expr_tree (vec<operand_entry *> *ops, gimple *stmt,
 		     bool is_associative, bool set_visited)
 {
@@ -4512,6 +4518,7 @@ 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);
+  bool changed = false;
 
   if (set_visited)
     gimple_set_visited (stmt, true);
@@ -4542,18 +4549,20 @@ linearize_expr_tree (vec<operand_entry *> *ops, gimple *stmt,
       if (!is_associative)
 	{
 	  add_to_ops_vec (ops, binrhs);
-	  return;
+	  return changed;
 	}
 
       if (!binrhsisreassoc)
 	{
-	  if (!try_special_add_to_ops (ops, rhscode, binrhs, binrhsdef))
+	  if (!try_special_add_to_ops (ops, rhscode, binrhs,
+				       binrhsdef, &changed))
 	    add_to_ops_vec (ops, binrhs);
 
-	  if (!try_special_add_to_ops (ops, rhscode, binlhs, binlhsdef))
+	  if (!try_special_add_to_ops (ops, rhscode, binlhs,
+				       binlhsdef, &changed))
 	    add_to_ops_vec (ops, binlhs);
 
-	  return;
+	  return changed;
 	}
 
       if (dump_file && (dump_flags & TDF_DETAILS))
@@ -4587,11 +4596,13 @@ linearize_expr_tree (vec<operand_entry *> *ops, gimple *stmt,
   gcc_assert (TREE_CODE (binrhs) != SSA_NAME
 	      || !is_reassociable_op (SSA_NAME_DEF_STMT (binrhs),
 				      rhscode, loop));
-  linearize_expr_tree (ops, SSA_NAME_DEF_STMT (binlhs),
-		       is_associative, set_visited);
+  changed |= linearize_expr_tree (ops, SSA_NAME_DEF_STMT (binlhs),
+				  is_associative, set_visited);
 
-  if (!try_special_add_to_ops (ops, rhscode, binrhs, binrhsdef))
+  if (!try_special_add_to_ops (ops, rhscode, binrhs,
+			       binrhsdef, &changed))
     add_to_ops_vec (ops, binrhs);
+  return changed;
 }
 
 /* Repropagate the negates back into subtracts, since no other pass
@@ -5250,6 +5261,7 @@ reassociate_bb (basic_block bb)
   basic_block son;
   gimple *stmt = last_stmt (bb);
   bool cfg_cleanup_needed = false;
+  bool changed = false;
 
   if (stmt && !gimple_visited_p (stmt))
     cfg_cleanup_needed |= maybe_optimize_range_tests (stmt);
@@ -5323,7 +5335,7 @@ reassociate_bb (basic_block bb)
 		continue;
 
 	      gimple_set_visited (stmt, true);
-	      linearize_expr_tree (&ops, stmt, true, true);
+	      changed = linearize_expr_tree (&ops, stmt, true, true);
 	      ops.qsort (sort_by_operand_rank);
 	      optimize_ops_list (rhs_code, &ops);
 	      if (undistribute_ops_list (rhs_code, &ops,
@@ -5415,7 +5427,7 @@ reassociate_bb (basic_block bb)
 
 		      new_lhs = rewrite_expr_tree (stmt, 0, ops,
 						   powi_result != NULL
-						   || negate_result);
+						   || changed);
                     }
 
 		  /* If we combined some repeated factors into a 

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[kugan]

On 10/08/16 08:51, kugan wrote:
> Hi Jakub,
>
> On 10/08/16 07:55, Jakub Jelinek wrote:
>> On Wed, Aug 10, 2016 at 07:51:08AM +1000, kugan wrote:
>>> On 10/08/16 07:46, Jakub Jelinek wrote:
>>>> On Wed, Aug 10, 2016 at 07:42:25AM +1000, kugan wrote:
>>>>> There was no new regression while testing. I also moved the testcase from
>>>>> gcc.dg/torture/pr72835.c to gcc.dg/tree-ssa/pr72835.c. Is this OK for trunk?
>>>>
>>>> This looks strange.  The tree-ssa-reassoc.c code has been trying to never
>>>> reuse SSA_NAMEs if they would hold a different value.
>>>> So there should be no resetting of flow sensitive info needed.
>>>
>>> We are not reusing but, if you see the example change in reassoc:
>>>
>>> -  _5 = -_4;
>>> -  _6 = _2 * _5;
>>> +  _5 = _4;
>>> +  _6 = _5 * _2;
>>>
>>> _5 and _6 will now have different value ranges because they compute
>>> different values. Therefore I think we should reset (?).
>>
>> No.  We should not have reused _5 and _6 for the different values.
>> It is not harmful just for the value ranges, but also for debug info.
>
> I see it now. The problem is we are just looking at (-1) being in the
> ops list for passing changed to rewrite_expr_tree in the case of
> multiplication by negate.  If we have combined (-1), as in the testcase,
> we will not have the (-1) and will pass changed=false to rewrite_expr_tree.
>
> We should set changed based on what happens in try_special_add_to_ops.
> Attached patch does this. Bootstrap and regression testing are ongoing.
> Is this OK for trunk if there is no regression.
>

This patch unfortunately does not handle all the cases. I am revising 
it. Sorry for the noise.

Thanks,
Kugan

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[Jakub Jelinek]

On Wed, Aug 10, 2016 at 08:51:32AM +1000, kugan wrote:
> I see it now. The problem is we are just looking at (-1) being in the ops
> list for passing changed to rewrite_expr_tree in the case of multiplication
> by negate.  If we have combined (-1), as in the testcase, we will not have
> the (-1) and will pass changed=false to rewrite_expr_tree.
> 
> We should set changed based on what happens in try_special_add_to_ops.
> Attached patch does this. Bootstrap and regression testing are ongoing. Is
> this OK for trunk if there is no regression.

I think the bug is elsewhere.  In particular in
undistribute_ops_list/zero_one_operation/decrement_power.
All those look problematic in this regard, they change RHS of statements
to something that holds a different value, while keeping the LHS.
So, generally you should instead just add a new stmt next to the old one,
and adjust data structures (replace the old SSA_NAME in some ->op with
the new one).  decrement_power might be a problem here, dunno if all the
builtins are const in all cases that DSE would kill the old one,
Richard, any preferences for that?  reset flow sensitive info + reset debug
stmt uses, or something different?  Though, replacing the LHS with a new
anonymous SSA_NAME might be needed too, in case it is before SSA_NAME of a
user var that doesn't yet have any debug stmts.

	Jakub

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[kugan]

[-- Attachment #1: Type: text/plain, Size: 1660 bytes --]



On 10/08/16 18:57, Jakub Jelinek wrote:
> On Wed, Aug 10, 2016 at 08:51:32AM +1000, kugan wrote:
>> I see it now. The problem is we are just looking at (-1) being in the ops
>> list for passing changed to rewrite_expr_tree in the case of multiplication
>> by negate.  If we have combined (-1), as in the testcase, we will not have
>> the (-1) and will pass changed=false to rewrite_expr_tree.
>>
>> We should set changed based on what happens in try_special_add_to_ops.
>> Attached patch does this. Bootstrap and regression testing are ongoing. Is
>> this OK for trunk if there is no regression.
>
> I think the bug is elsewhere.  In particular in
> undistribute_ops_list/zero_one_operation/decrement_power.
> All those look problematic in this regard, they change RHS of statements
> to something that holds a different value, while keeping the LHS.
> So, generally you should instead just add a new stmt next to the old one,
> and adjust data structures (replace the old SSA_NAME in some ->op with
> the new one).  decrement_power might be a problem here, dunno if all the
> builtins are const in all cases that DSE would kill the old one,
> Richard, any preferences for that?  reset flow sensitive info + reset debug
> stmt uses, or something different?  Though, replacing the LHS with a new
> anonymous SSA_NAME might be needed too, in case it is before SSA_NAME of a
> user var that doesn't yet have any debug stmts.

Hi Jakub,

This is the patch I have now (not full tested yet). This is along what 
you described above. I think I have to handle all the LHS in 
zero_one_operation too, I will wait for the feedback before working on it.

Thanks,
Kugan

[-- Attachment #2: pr72835_3.txt --]
[-- Type: text/plain, Size: 8403 bytes --]

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c b/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
index e69de29..049eddc 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
@@ -0,0 +1,36 @@
+/* PR tree-optimization/72835.  */
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+struct struct_1 {
+    unsigned int m1 : 6 ;
+    unsigned int m2 : 24 ;
+    unsigned int m3 : 6 ;
+};
+
+unsigned short var_32 = 0x2d10;
+
+struct struct_1 s1;
+
+void init ()
+{
+  s1.m1 = 4;
+  s1.m2 = 0x7ca4b8;
+  s1.m3 = 24;
+}
+
+void foo ()
+{
+  unsigned int c
+    = ((unsigned int) s1.m2) * (-((unsigned int) s1.m3))
+    + (var_32) * (-((unsigned int) (s1.m1)));
+  if (c != 4098873984)
+    __builtin_abort ();
+}
+
+int main ()
+{
+    init ();
+    foo ();
+    return 0;
+}
diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
index 7fd7550..b8dfa39 100644
--- a/gcc/tree-ssa-reassoc.c
+++ b/gcc/tree-ssa-reassoc.c
@@ -1039,7 +1039,7 @@ eliminate_using_constants (enum tree_code opcode,
 
 
 static void linearize_expr_tree (vec<operand_entry *> *, gimple *,
-				 bool, bool);
+				 bool, bool, bool *);
 
 /* Structure for tracking and counting operands.  */
 struct oecount {
@@ -1183,9 +1183,25 @@ propagate_op_to_single_use (tree op, gimple *stmt, tree *def)
    is updated if there is only one operand but no operation left.  */
 
 static void
-zero_one_operation (tree *def, enum tree_code opcode, tree op)
+zero_one_operation (tree *def, enum tree_code opcode, tree op, bool ops_changed)
 {
   gimple *stmt = SSA_NAME_DEF_STMT (*def);
+  /* In this case, the result in the *def will be different as
+     compared to how it was.  Therefore, to avoid having SSA
+     which will have range_info and debug that reflects old
+     operation, create a new SSA and use it (PR72835).  */
+  if (ops_changed)
+    {
+      use_operand_p use_p;
+      gimple *use_stmt;
+      gimple *stmt = SSA_NAME_DEF_STMT (*def);
+      tree new_def = make_ssa_name (TREE_TYPE (*def));
+      gcc_checking_assert (single_imm_use (*def, &use_p, &use_stmt));
+      SET_USE (use_p, new_def);
+      gimple_set_lhs (stmt, new_def);
+      *def = new_def;
+      update_stmt (use_stmt);
+    }
 
   do
     {
@@ -1250,6 +1266,15 @@ zero_one_operation (tree *def, enum tree_code opcode, tree op)
 	  else if (is_gimple_assign (stmt2)
 		   && gimple_assign_rhs_code (stmt2) == NEGATE_EXPR)
 	    {
+	      /* In this case the result in the op will be
+		 different as compared to how it was.  Therefore, to avoid
+		 having SSA which will have range_info and debug that
+		 reflects old operation, create a new SSA and use
+		 it (PR72835).  */
+	      tree tmp = make_ssa_name (TREE_TYPE (op));
+	      gimple_set_lhs (stmt2, tmp);
+	      gimple_assign_set_rhs2 (stmt, tmp);
+	      update_stmt (stmt2);
 	      if (gimple_assign_rhs1 (stmt2) == op)
 		{
 		  tree cst = build_minus_one_cst (TREE_TYPE (op));
@@ -1453,7 +1478,8 @@ build_and_add_sum (tree type, tree op1, tree op2, enum tree_code opcode)
 
 static bool
 undistribute_ops_list (enum tree_code opcode,
-		       vec<operand_entry *> *ops, struct loop *loop)
+		       vec<operand_entry *> *ops, struct loop *loop,
+		       bool *ops_changed)
 {
   unsigned int length = ops->length ();
   operand_entry *oe1;
@@ -1521,7 +1547,7 @@ undistribute_ops_list (enum tree_code opcode,
       oedef = SSA_NAME_DEF_STMT ((*ops)[i]->op);
       oecode = gimple_assign_rhs_code (oedef);
       linearize_expr_tree (&subops[i], oedef,
-			   associative_tree_code (oecode), false);
+			   associative_tree_code (oecode), false, ops_changed);
 
       FOR_EACH_VEC_ELT (subops[i], j, oe1)
 	{
@@ -1617,7 +1643,7 @@ undistribute_ops_list (enum tree_code opcode,
 	      fprintf (dump_file, "Building (");
 	      print_generic_expr (dump_file, oe1->op, 0);
 	    }
-	  zero_one_operation (&oe1->op, c->oecode, c->op);
+	  zero_one_operation (&oe1->op, c->oecode, c->op, *ops_changed);
 	  EXECUTE_IF_SET_IN_BITMAP (candidates2, first+1, i, sbi0)
 	    {
 	      gimple *sum;
@@ -1627,7 +1653,7 @@ undistribute_ops_list (enum tree_code opcode,
 		  fprintf (dump_file, " + ");
 		  print_generic_expr (dump_file, oe2->op, 0);
 		}
-	      zero_one_operation (&oe2->op, c->oecode, c->op);
+	      zero_one_operation (&oe2->op, c->oecode, c->op, *ops_changed);
 	      sum = build_and_add_sum (TREE_TYPE (oe1->op),
 				       oe1->op, oe2->op, opcode);
 	      oe2->op = build_zero_cst (TREE_TYPE (oe2->op));
@@ -4456,12 +4482,16 @@ acceptable_pow_call (gcall *stmt, tree *base, HOST_WIDE_INT *exponent)
 }
 
 /* Try to derive and add operand entry for OP to *OPS.  Return false if
-   unsuccessful.  */
+   unsuccessful.  If we changed the operands such that the (intermediate)
+   results can be different (as in the case of NEGATE_EXPR converted to
+   multiplication by -1), set ops_changed to true so that we will not
+   reuse the SSA (PR72835).  */
 
 static bool
 try_special_add_to_ops (vec<operand_entry *> *ops,
 			enum tree_code code,
-			tree op, gimple* def_stmt)
+			tree op, gimple* def_stmt,
+			bool *ops_changed)
 {
   tree base = NULL_TREE;
   HOST_WIDE_INT exponent = 0;
@@ -4492,6 +4522,8 @@ try_special_add_to_ops (vec<operand_entry *> *ops,
       add_to_ops_vec (ops, rhs1);
       add_to_ops_vec (ops, cst);
       gimple_set_visited (def_stmt, true);
+      if (ops_changed)
+	*ops_changed = true;
       return true;
     }
 
@@ -4499,11 +4531,12 @@ try_special_add_to_ops (vec<operand_entry *> *ops,
 }
 
 /* Recursively linearize a binary expression that is the RHS of STMT.
-   Place the operands of the expression tree in the vector named OPS.  */
+   Place the operands of the expression tree in the vector named OPS.
+   Return TRUE if try_special_add_to_ops has set ops_changed to TRUE.  */
 
 static void
 linearize_expr_tree (vec<operand_entry *> *ops, gimple *stmt,
-		     bool is_associative, bool set_visited)
+		     bool is_associative, bool set_visited, bool *ops_changed)
 {
   tree binlhs = gimple_assign_rhs1 (stmt);
   tree binrhs = gimple_assign_rhs2 (stmt);
@@ -4547,10 +4580,12 @@ linearize_expr_tree (vec<operand_entry *> *ops, gimple *stmt,
 
       if (!binrhsisreassoc)
 	{
-	  if (!try_special_add_to_ops (ops, rhscode, binrhs, binrhsdef))
+	  if (!try_special_add_to_ops (ops, rhscode, binrhs,
+				       binrhsdef, ops_changed))
 	    add_to_ops_vec (ops, binrhs);
 
-	  if (!try_special_add_to_ops (ops, rhscode, binlhs, binlhsdef))
+	  if (!try_special_add_to_ops (ops, rhscode, binlhs,
+				       binlhsdef, ops_changed))
 	    add_to_ops_vec (ops, binlhs);
 
 	  return;
@@ -4588,9 +4623,9 @@ linearize_expr_tree (vec<operand_entry *> *ops, gimple *stmt,
 	      || !is_reassociable_op (SSA_NAME_DEF_STMT (binrhs),
 				      rhscode, loop));
   linearize_expr_tree (ops, SSA_NAME_DEF_STMT (binlhs),
-		       is_associative, set_visited);
+		       is_associative, set_visited, ops_changed);
 
-  if (!try_special_add_to_ops (ops, rhscode, binrhs, binrhsdef))
+  if (!try_special_add_to_ops (ops, rhscode, binrhs, binrhsdef, ops_changed))
     add_to_ops_vec (ops, binrhs);
 }
 
@@ -5322,12 +5357,20 @@ reassociate_bb (basic_block bb)
 	      if (TREE_CODE (lhs) == SSA_NAME && has_zero_uses (lhs))
 		continue;
 
+	      bool ops_changed = false;
 	      gimple_set_visited (stmt, true);
-	      linearize_expr_tree (&ops, stmt, true, true);
+	      linearize_expr_tree (&ops, stmt, true, true, NULL);
 	      ops.qsort (sort_by_operand_rank);
 	      optimize_ops_list (rhs_code, &ops);
+	      /* While in undistribute_ops_list, NEGATE_EXPR is factored out,
+		 operands to the reassociated stmts will be different
+		 compared to how it was. In this case, to avoid having SSA
+		 which will have range_info and debug that reflects old
+		 operation, rewrite_expr_tree has to be called with
+		 changed = true (PR72835).  */
 	      if (undistribute_ops_list (rhs_code, &ops,
-					 loop_containing_stmt (stmt)))
+					 loop_containing_stmt (stmt),
+					 &ops_changed))
 		{
 		  ops.qsort (sort_by_operand_rank);
 		  optimize_ops_list (rhs_code, &ops);
@@ -5415,7 +5458,8 @@ reassociate_bb (basic_block bb)
 
 		      new_lhs = rewrite_expr_tree (stmt, 0, ops,
 						   powi_result != NULL
-						   || negate_result);
+						   || negate_result
+						   || ops_changed);
                     }
 
 		  /* If we combined some repeated factors into a 

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[Richard Biener]

On Wed, Aug 10, 2016 at 10:57 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Aug 10, 2016 at 08:51:32AM +1000, kugan wrote:
>> I see it now. The problem is we are just looking at (-1) being in the ops
>> list for passing changed to rewrite_expr_tree in the case of multiplication
>> by negate.  If we have combined (-1), as in the testcase, we will not have
>> the (-1) and will pass changed=false to rewrite_expr_tree.
>>
>> We should set changed based on what happens in try_special_add_to_ops.
>> Attached patch does this. Bootstrap and regression testing are ongoing. Is
>> this OK for trunk if there is no regression.
>
> I think the bug is elsewhere.  In particular in
> undistribute_ops_list/zero_one_operation/decrement_power.
> All those look problematic in this regard, they change RHS of statements
> to something that holds a different value, while keeping the LHS.
> So, generally you should instead just add a new stmt next to the old one,
> and adjust data structures (replace the old SSA_NAME in some ->op with
> the new one).  decrement_power might be a problem here, dunno if all the
> builtins are const in all cases that DSE would kill the old one,
> Richard, any preferences for that?  reset flow sensitive info + reset debug
> stmt uses, or something different?  Though, replacing the LHS with a new
> anonymous SSA_NAME might be needed too, in case it is before SSA_NAME of a
> user var that doesn't yet have any debug stmts.

I'd say replacing the LHS is the way to go, with calling the appropriate helper
on the old stmt to generate a debug stmt for it / its uses (would need
to look it
up here).

Richard.

>
>         Jakub

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[kugan]

[-- Attachment #1: Type: text/plain, Size: 3522 bytes --]

Hi,

On 10/08/16 20:28, Richard Biener wrote:
> On Wed, Aug 10, 2016 at 10:57 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Wed, Aug 10, 2016 at 08:51:32AM +1000, kugan wrote:
>>> I see it now. The problem is we are just looking at (-1) being in the ops
>>> list for passing changed to rewrite_expr_tree in the case of multiplication
>>> by negate.  If we have combined (-1), as in the testcase, we will not have
>>> the (-1) and will pass changed=false to rewrite_expr_tree.
>>>
>>> We should set changed based on what happens in try_special_add_to_ops.
>>> Attached patch does this. Bootstrap and regression testing are ongoing. Is
>>> this OK for trunk if there is no regression.
>>
>> I think the bug is elsewhere.  In particular in
>> undistribute_ops_list/zero_one_operation/decrement_power.
>> All those look problematic in this regard, they change RHS of statements
>> to something that holds a different value, while keeping the LHS.
>> So, generally you should instead just add a new stmt next to the old one,
>> and adjust data structures (replace the old SSA_NAME in some ->op with
>> the new one).  decrement_power might be a problem here, dunno if all the
>> builtins are const in all cases that DSE would kill the old one,
>> Richard, any preferences for that?  reset flow sensitive info + reset debug
>> stmt uses, or something different?  Though, replacing the LHS with a new
>> anonymous SSA_NAME might be needed too, in case it is before SSA_NAME of a
>> user var that doesn't yet have any debug stmts.
>
> I'd say replacing the LHS is the way to go, with calling the appropriate helper
> on the old stmt to generate a debug stmt for it / its uses (would need
> to look it
> up here).
>

Here is an attempt to fix it. The problem arises when in 
undistribute_ops_list, we linearize_expr_tree such that NEGATE_EXPR is 
added (-1) MULT_EXPR (OP). Real problem starts when we handle this in 
zero_one_operation. Unlike what was done earlier, we now change the stmt 
(with propagate_op_to_signle use or by directly) such that the value 
computed by stmt is no longer what it used to be. Because of this, what 
is computed in undistribute_ops_list and rewrite_expr_tree are also changed.

undistribute_ops_list already expects this but rewrite_expr_tree will 
not if we dont pass the changed as an argument.

The way I am fixing this now is, in linearize_expr_tree, I set 
ops_changed  to true if we change NEGATE_EXPR to (-1) MULT_EXPR (OP). 
Then when we call zero_one_operation with ops_changed = true, I replace 
all the LHS in zero_one_operation with the new SSA and replace all the 
uses. I also call the rewrite_expr_tree with changed = false in this case.

Does this make sense? Bootstrapped and regression tested for 
x86_64-linux-gnu without any new regressions.

Thanks,
Kugan


gcc/testsuite/ChangeLog:

2016-08-10  Kugan Vivekanandarajah  <kuganv@linaro.org>

	PR tree-optimization/72835
	* gcc.dg/tree-ssa/pr72835.c: New test.

gcc/ChangeLog:

2016-08-10  Kugan Vivekanandarajah  <kuganv@linaro.org>

	PR tree-optimization/72835
	* tree-ssa-reassoc.c (zero_one_operation): Incase of NEGATE_EXPR create 
and use
	 new SSA_NAME.
	(try_special_add_to_ops): Return true if we changed the value in operands.
	(linearize_expr_tree): Return true if try_special_add_top_ops set 
ops_changed to true.
	(undistribute_ops_list): Likewise.
	(reassociate_bb): Pass ops_changed returned by linearlize_expr_tree to 
rewrite_expr_tree.



whil cif we change the operands such that the

/zero_one_operation

[-- Attachment #2: p.txt --]
[-- Type: text/plain, Size: 8981 bytes --]

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c b/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
index e69de29..049eddc 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
@@ -0,0 +1,36 @@
+/* PR tree-optimization/72835.  */
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+struct struct_1 {
+    unsigned int m1 : 6 ;
+    unsigned int m2 : 24 ;
+    unsigned int m3 : 6 ;
+};
+
+unsigned short var_32 = 0x2d10;
+
+struct struct_1 s1;
+
+void init ()
+{
+  s1.m1 = 4;
+  s1.m2 = 0x7ca4b8;
+  s1.m3 = 24;
+}
+
+void foo ()
+{
+  unsigned int c
+    = ((unsigned int) s1.m2) * (-((unsigned int) s1.m3))
+    + (var_32) * (-((unsigned int) (s1.m1)));
+  if (c != 4098873984)
+    __builtin_abort ();
+}
+
+int main ()
+{
+    init ();
+    foo ();
+    return 0;
+}
diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
index 7fd7550..038da41 100644
--- a/gcc/tree-ssa-reassoc.c
+++ b/gcc/tree-ssa-reassoc.c
@@ -1039,7 +1039,7 @@ eliminate_using_constants (enum tree_code opcode,
 
 
 static void linearize_expr_tree (vec<operand_entry *> *, gimple *,
-				 bool, bool);
+				 bool, bool, bool *);
 
 /* Structure for tracking and counting operands.  */
 struct oecount {
@@ -1183,7 +1183,7 @@ propagate_op_to_single_use (tree op, gimple *stmt, tree *def)
    is updated if there is only one operand but no operation left.  */
 
 static void
-zero_one_operation (tree *def, enum tree_code opcode, tree op)
+zero_one_operation (tree *def, enum tree_code opcode, tree op, bool ops_changed)
 {
   gimple *stmt = SSA_NAME_DEF_STMT (*def);
 
@@ -1193,6 +1193,27 @@ zero_one_operation (tree *def, enum tree_code opcode, tree op)
 
       if (opcode == MULT_EXPR)
 	{
+	  /* In this case, the result in the *def will be different as
+	     compared to how it was.  Therefore, to avoid having SSA
+	     which will have range_info and debug that reflects old
+	     operation, create a new SSA and use it (PR72835).  */
+	  if (ops_changed)
+	    {
+	      imm_use_iterator iter;
+	      use_operand_p use_p;
+	      gimple *use_stmt;
+	      tree lhs = gimple_assign_lhs (stmt);
+	      tree new_lhs = make_ssa_name (TREE_TYPE (lhs));
+	      FOR_EACH_IMM_USE_STMT (use_stmt, iter, lhs)
+		{
+		  FOR_EACH_IMM_USE_ON_STMT (use_p, iter)
+		    SET_USE (use_p, new_lhs);
+		  update_stmt (use_stmt);
+		}
+	      if (*def == lhs)
+		*def = new_lhs;
+	      gimple_set_lhs (stmt, new_lhs);
+	    }
 	  if (stmt_is_power_of_op (stmt, op))
 	    {
 	      if (decrement_power (stmt) == 1)
@@ -1241,6 +1262,26 @@ zero_one_operation (tree *def, enum tree_code opcode, tree op)
 	  && has_single_use (gimple_assign_rhs2 (stmt)))
 	{
 	  gimple *stmt2 = SSA_NAME_DEF_STMT (gimple_assign_rhs2 (stmt));
+	  /* In this case the result in the op will be
+	     different as compared to how it was.  Therefore, to avoid
+	     having SSA which will have range_info and debug that
+	     reflects old operation, create a new SSA and use
+	     it (PR72835).  */
+	  if (ops_changed)
+	    {
+	      imm_use_iterator iter;
+	      use_operand_p use_p;
+	      gimple *use_stmt;
+	      tree lhs = gimple_assign_lhs (stmt2);
+	      tree new_lhs = make_ssa_name (TREE_TYPE (lhs));
+	      FOR_EACH_IMM_USE_STMT (use_stmt, iter, lhs)
+		{
+		  FOR_EACH_IMM_USE_ON_STMT (use_p, iter)
+		    SET_USE (use_p, new_lhs);
+		  update_stmt (use_stmt);
+		}
+	      gimple_set_lhs (stmt2, new_lhs);
+	    }
 	  if (stmt_is_power_of_op (stmt2, op))
 	    {
 	      if (decrement_power (stmt2) == 1)
@@ -1453,7 +1494,8 @@ build_and_add_sum (tree type, tree op1, tree op2, enum tree_code opcode)
 
 static bool
 undistribute_ops_list (enum tree_code opcode,
-		       vec<operand_entry *> *ops, struct loop *loop)
+		       vec<operand_entry *> *ops, struct loop *loop,
+		       bool *ops_changed)
 {
   unsigned int length = ops->length ();
   operand_entry *oe1;
@@ -1521,7 +1563,7 @@ undistribute_ops_list (enum tree_code opcode,
       oedef = SSA_NAME_DEF_STMT ((*ops)[i]->op);
       oecode = gimple_assign_rhs_code (oedef);
       linearize_expr_tree (&subops[i], oedef,
-			   associative_tree_code (oecode), false);
+			   associative_tree_code (oecode), false, ops_changed);
 
       FOR_EACH_VEC_ELT (subops[i], j, oe1)
 	{
@@ -1617,7 +1659,7 @@ undistribute_ops_list (enum tree_code opcode,
 	      fprintf (dump_file, "Building (");
 	      print_generic_expr (dump_file, oe1->op, 0);
 	    }
-	  zero_one_operation (&oe1->op, c->oecode, c->op);
+	  zero_one_operation (&oe1->op, c->oecode, c->op, *ops_changed);
 	  EXECUTE_IF_SET_IN_BITMAP (candidates2, first+1, i, sbi0)
 	    {
 	      gimple *sum;
@@ -1627,7 +1669,7 @@ undistribute_ops_list (enum tree_code opcode,
 		  fprintf (dump_file, " + ");
 		  print_generic_expr (dump_file, oe2->op, 0);
 		}
-	      zero_one_operation (&oe2->op, c->oecode, c->op);
+	      zero_one_operation (&oe2->op, c->oecode, c->op, *ops_changed);
 	      sum = build_and_add_sum (TREE_TYPE (oe1->op),
 				       oe1->op, oe2->op, opcode);
 	      oe2->op = build_zero_cst (TREE_TYPE (oe2->op));
@@ -4456,12 +4498,16 @@ acceptable_pow_call (gcall *stmt, tree *base, HOST_WIDE_INT *exponent)
 }
 
 /* Try to derive and add operand entry for OP to *OPS.  Return false if
-   unsuccessful.  */
+   unsuccessful.  If we changed the operands such that the (intermediate)
+   results can be different (as in the case of NEGATE_EXPR converted to
+   multiplication by -1), set ops_changed to true so that we will not
+   reuse the SSA (PR72835).  */
 
 static bool
 try_special_add_to_ops (vec<operand_entry *> *ops,
 			enum tree_code code,
-			tree op, gimple* def_stmt)
+			tree op, gimple* def_stmt,
+			bool *ops_changed)
 {
   tree base = NULL_TREE;
   HOST_WIDE_INT exponent = 0;
@@ -4492,6 +4538,8 @@ try_special_add_to_ops (vec<operand_entry *> *ops,
       add_to_ops_vec (ops, rhs1);
       add_to_ops_vec (ops, cst);
       gimple_set_visited (def_stmt, true);
+      if (ops_changed)
+	*ops_changed = true;
       return true;
     }
 
@@ -4499,11 +4547,12 @@ try_special_add_to_ops (vec<operand_entry *> *ops,
 }
 
 /* Recursively linearize a binary expression that is the RHS of STMT.
-   Place the operands of the expression tree in the vector named OPS.  */
+   Place the operands of the expression tree in the vector named OPS.
+   Return TRUE if try_special_add_to_ops has set ops_changed to TRUE.  */
 
 static void
 linearize_expr_tree (vec<operand_entry *> *ops, gimple *stmt,
-		     bool is_associative, bool set_visited)
+		     bool is_associative, bool set_visited, bool *ops_changed)
 {
   tree binlhs = gimple_assign_rhs1 (stmt);
   tree binrhs = gimple_assign_rhs2 (stmt);
@@ -4547,10 +4596,12 @@ linearize_expr_tree (vec<operand_entry *> *ops, gimple *stmt,
 
       if (!binrhsisreassoc)
 	{
-	  if (!try_special_add_to_ops (ops, rhscode, binrhs, binrhsdef))
+	  if (!try_special_add_to_ops (ops, rhscode, binrhs,
+				       binrhsdef, ops_changed))
 	    add_to_ops_vec (ops, binrhs);
 
-	  if (!try_special_add_to_ops (ops, rhscode, binlhs, binlhsdef))
+	  if (!try_special_add_to_ops (ops, rhscode, binlhs,
+				       binlhsdef, ops_changed))
 	    add_to_ops_vec (ops, binlhs);
 
 	  return;
@@ -4588,9 +4639,9 @@ linearize_expr_tree (vec<operand_entry *> *ops, gimple *stmt,
 	      || !is_reassociable_op (SSA_NAME_DEF_STMT (binrhs),
 				      rhscode, loop));
   linearize_expr_tree (ops, SSA_NAME_DEF_STMT (binlhs),
-		       is_associative, set_visited);
+		       is_associative, set_visited, ops_changed);
 
-  if (!try_special_add_to_ops (ops, rhscode, binrhs, binrhsdef))
+  if (!try_special_add_to_ops (ops, rhscode, binrhs, binrhsdef, ops_changed))
     add_to_ops_vec (ops, binrhs);
 }
 
@@ -5322,12 +5373,20 @@ reassociate_bb (basic_block bb)
 	      if (TREE_CODE (lhs) == SSA_NAME && has_zero_uses (lhs))
 		continue;
 
+	      bool ops_changed = false;
 	      gimple_set_visited (stmt, true);
-	      linearize_expr_tree (&ops, stmt, true, true);
+	      linearize_expr_tree (&ops, stmt, true, true, NULL);
 	      ops.qsort (sort_by_operand_rank);
 	      optimize_ops_list (rhs_code, &ops);
+	      /* While in undistribute_ops_list, NEGATE_EXPR is factored out,
+		 operands to the reassociated stmts will be different
+		 compared to how it was. In this case, to avoid having SSA
+		 which will have range_info and debug that reflects old
+		 operation, rewrite_expr_tree has to be called with
+		 changed = true (PR72835).  */
 	      if (undistribute_ops_list (rhs_code, &ops,
-					 loop_containing_stmt (stmt)))
+					 loop_containing_stmt (stmt),
+					 &ops_changed))
 		{
 		  ops.qsort (sort_by_operand_rank);
 		  optimize_ops_list (rhs_code, &ops);
@@ -5415,7 +5474,8 @@ reassociate_bb (basic_block bb)
 
 		      new_lhs = rewrite_expr_tree (stmt, 0, ops,
 						   powi_result != NULL
-						   || negate_result);
+						   || negate_result
+						   || ops_changed);
                     }
 
 		  /* If we combined some repeated factors into a 

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[Kugan Vivekanandarajah]

Ping?

https://gcc.gnu.org/ml/gcc-patches/2016-08/msg00872.html

Thanks,
Kugan

On 11 August 2016 at 09:09, kugan <kugan.vivekanandarajah@linaro.org> wrote:
> Hi,
>
>
> On 10/08/16 20:28, Richard Biener wrote:
>>
>> On Wed, Aug 10, 2016 at 10:57 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>
>>> On Wed, Aug 10, 2016 at 08:51:32AM +1000, kugan wrote:
>>>>
>>>> I see it now. The problem is we are just looking at (-1) being in the
>>>> ops
>>>> list for passing changed to rewrite_expr_tree in the case of
>>>> multiplication
>>>> by negate.  If we have combined (-1), as in the testcase, we will not
>>>> have
>>>> the (-1) and will pass changed=false to rewrite_expr_tree.
>>>>
>>>> We should set changed based on what happens in try_special_add_to_ops.
>>>> Attached patch does this. Bootstrap and regression testing are ongoing.
>>>> Is
>>>> this OK for trunk if there is no regression.
>>>
>>>
>>> I think the bug is elsewhere.  In particular in
>>> undistribute_ops_list/zero_one_operation/decrement_power.
>>> All those look problematic in this regard, they change RHS of statements
>>> to something that holds a different value, while keeping the LHS.
>>> So, generally you should instead just add a new stmt next to the old one,
>>> and adjust data structures (replace the old SSA_NAME in some ->op with
>>> the new one).  decrement_power might be a problem here, dunno if all the
>>> builtins are const in all cases that DSE would kill the old one,
>>> Richard, any preferences for that?  reset flow sensitive info + reset
>>> debug
>>> stmt uses, or something different?  Though, replacing the LHS with a new
>>> anonymous SSA_NAME might be needed too, in case it is before SSA_NAME of
>>> a
>>> user var that doesn't yet have any debug stmts.
>>
>>
>> I'd say replacing the LHS is the way to go, with calling the appropriate
>> helper
>> on the old stmt to generate a debug stmt for it / its uses (would need
>> to look it
>> up here).
>>
>
> Here is an attempt to fix it. The problem arises when in
> undistribute_ops_list, we linearize_expr_tree such that NEGATE_EXPR is added
> (-1) MULT_EXPR (OP). Real problem starts when we handle this in
> zero_one_operation. Unlike what was done earlier, we now change the stmt
> (with propagate_op_to_signle use or by directly) such that the value
> computed by stmt is no longer what it used to be. Because of this, what is
> computed in undistribute_ops_list and rewrite_expr_tree are also changed.
>
> undistribute_ops_list already expects this but rewrite_expr_tree will not if
> we dont pass the changed as an argument.
>
> The way I am fixing this now is, in linearize_expr_tree, I set ops_changed
> to true if we change NEGATE_EXPR to (-1) MULT_EXPR (OP). Then when we call
> zero_one_operation with ops_changed = true, I replace all the LHS in
> zero_one_operation with the new SSA and replace all the uses. I also call
> the rewrite_expr_tree with changed = false in this case.
>
> Does this make sense? Bootstrapped and regression tested for
> x86_64-linux-gnu without any new regressions.
>
> Thanks,
> Kugan
>
>
> gcc/testsuite/ChangeLog:
>
> 2016-08-10  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>         PR tree-optimization/72835
>         * gcc.dg/tree-ssa/pr72835.c: New test.
>
> gcc/ChangeLog:
>
> 2016-08-10  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>         PR tree-optimization/72835
>         * tree-ssa-reassoc.c (zero_one_operation): Incase of NEGATE_EXPR
> create and use
>          new SSA_NAME.
>         (try_special_add_to_ops): Return true if we changed the value in
> operands.
>         (linearize_expr_tree): Return true if try_special_add_top_ops set
> ops_changed to true.
>         (undistribute_ops_list): Likewise.
>         (reassociate_bb): Pass ops_changed returned by linearlize_expr_tree
> to rewrite_expr_tree.
>
>
>
> whil cif we change the operands such that the
>
> /zero_one_operation

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[Richard Biener]

On Thu, Aug 11, 2016 at 1:09 AM, kugan
<kugan.vivekanandarajah@linaro.org> wrote:
> Hi,
>
>
> On 10/08/16 20:28, Richard Biener wrote:
>>
>> On Wed, Aug 10, 2016 at 10:57 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>
>>> On Wed, Aug 10, 2016 at 08:51:32AM +1000, kugan wrote:
>>>>
>>>> I see it now. The problem is we are just looking at (-1) being in the
>>>> ops
>>>> list for passing changed to rewrite_expr_tree in the case of
>>>> multiplication
>>>> by negate.  If we have combined (-1), as in the testcase, we will not
>>>> have
>>>> the (-1) and will pass changed=false to rewrite_expr_tree.
>>>>
>>>> We should set changed based on what happens in try_special_add_to_ops.
>>>> Attached patch does this. Bootstrap and regression testing are ongoing.
>>>> Is
>>>> this OK for trunk if there is no regression.
>>>
>>>
>>> I think the bug is elsewhere.  In particular in
>>> undistribute_ops_list/zero_one_operation/decrement_power.
>>> All those look problematic in this regard, they change RHS of statements
>>> to something that holds a different value, while keeping the LHS.
>>> So, generally you should instead just add a new stmt next to the old one,
>>> and adjust data structures (replace the old SSA_NAME in some ->op with
>>> the new one).  decrement_power might be a problem here, dunno if all the
>>> builtins are const in all cases that DSE would kill the old one,
>>> Richard, any preferences for that?  reset flow sensitive info + reset
>>> debug
>>> stmt uses, or something different?  Though, replacing the LHS with a new
>>> anonymous SSA_NAME might be needed too, in case it is before SSA_NAME of
>>> a
>>> user var that doesn't yet have any debug stmts.
>>
>>
>> I'd say replacing the LHS is the way to go, with calling the appropriate
>> helper
>> on the old stmt to generate a debug stmt for it / its uses (would need
>> to look it
>> up here).
>>
>
> Here is an attempt to fix it. The problem arises when in
> undistribute_ops_list, we linearize_expr_tree such that NEGATE_EXPR is added
> (-1) MULT_EXPR (OP). Real problem starts when we handle this in
> zero_one_operation. Unlike what was done earlier, we now change the stmt
> (with propagate_op_to_signle use or by directly) such that the value
> computed by stmt is no longer what it used to be. Because of this, what is
> computed in undistribute_ops_list and rewrite_expr_tree are also changed.
>
> undistribute_ops_list already expects this but rewrite_expr_tree will not if
> we dont pass the changed as an argument.
>
> The way I am fixing this now is, in linearize_expr_tree, I set ops_changed
> to true if we change NEGATE_EXPR to (-1) MULT_EXPR (OP). Then when we call
> zero_one_operation with ops_changed = true, I replace all the LHS in
> zero_one_operation with the new SSA and replace all the uses. I also call
> the rewrite_expr_tree with changed = false in this case.
>
> Does this make sense? Bootstrapped and regression tested for
> x86_64-linux-gnu without any new regressions.

I don't think this solves the issue.  zero_one_operation associates the
chain starting at the first *def and it will change the intermediate values
of _all_ of the stmts visited until the operation to be removed is found.
Note that this is independent of whether try_special_add_to_ops did anything.

Even for the regular undistribution cases we get this wrong.

So we need to back-track in zero_one_operation, replacing each LHS
and in the end the op in the opvector of the main chain.  That's basically
the same as if we'd do a regular re-assoc operation on the sub-chains.
Take their subops, simulate zero_one_operation by
appending the cancelling operation and optimizing the oplist, and then
materializing the associated ops via rewrite_expr_tree.

Richard.

> Thanks,
> Kugan
>
>
> gcc/testsuite/ChangeLog:
>
> 2016-08-10  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>         PR tree-optimization/72835
>         * gcc.dg/tree-ssa/pr72835.c: New test.
>
> gcc/ChangeLog:
>
> 2016-08-10  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>         PR tree-optimization/72835
>         * tree-ssa-reassoc.c (zero_one_operation): Incase of NEGATE_EXPR
> create and use
>          new SSA_NAME.
>         (try_special_add_to_ops): Return true if we changed the value in
> operands.
>         (linearize_expr_tree): Return true if try_special_add_top_ops set
> ops_changed to true.
>         (undistribute_ops_list): Likewise.
>         (reassociate_bb): Pass ops_changed returned by linearlize_expr_tree
> to rewrite_expr_tree.
>
>
>
> whil cif we change the operands such that the
>
> /zero_one_operation

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[Kugan Vivekanandarajah]

[-- Attachment #1: Type: text/plain, Size: 4213 bytes --]

Hi Richard,

On 25 August 2016 at 22:24, Richard Biener <richard.guenther@gmail.com> wrote:
> On Thu, Aug 11, 2016 at 1:09 AM, kugan
> <kugan.vivekanandarajah@linaro.org> wrote:
>> Hi,
>>
>>
>> On 10/08/16 20:28, Richard Biener wrote:
>>>
>>> On Wed, Aug 10, 2016 at 10:57 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>
>>>> On Wed, Aug 10, 2016 at 08:51:32AM +1000, kugan wrote:
>>>>>
>>>>> I see it now. The problem is we are just looking at (-1) being in the
>>>>> ops
>>>>> list for passing changed to rewrite_expr_tree in the case of
>>>>> multiplication
>>>>> by negate.  If we have combined (-1), as in the testcase, we will not
>>>>> have
>>>>> the (-1) and will pass changed=false to rewrite_expr_tree.
>>>>>
>>>>> We should set changed based on what happens in try_special_add_to_ops.
>>>>> Attached patch does this. Bootstrap and regression testing are ongoing.
>>>>> Is
>>>>> this OK for trunk if there is no regression.
>>>>
>>>>
>>>> I think the bug is elsewhere.  In particular in
>>>> undistribute_ops_list/zero_one_operation/decrement_power.
>>>> All those look problematic in this regard, they change RHS of statements
>>>> to something that holds a different value, while keeping the LHS.
>>>> So, generally you should instead just add a new stmt next to the old one,
>>>> and adjust data structures (replace the old SSA_NAME in some ->op with
>>>> the new one).  decrement_power might be a problem here, dunno if all the
>>>> builtins are const in all cases that DSE would kill the old one,
>>>> Richard, any preferences for that?  reset flow sensitive info + reset
>>>> debug
>>>> stmt uses, or something different?  Though, replacing the LHS with a new
>>>> anonymous SSA_NAME might be needed too, in case it is before SSA_NAME of
>>>> a
>>>> user var that doesn't yet have any debug stmts.
>>>
>>>
>>> I'd say replacing the LHS is the way to go, with calling the appropriate
>>> helper
>>> on the old stmt to generate a debug stmt for it / its uses (would need
>>> to look it
>>> up here).
>>>
>>
>> Here is an attempt to fix it. The problem arises when in
>> undistribute_ops_list, we linearize_expr_tree such that NEGATE_EXPR is added
>> (-1) MULT_EXPR (OP). Real problem starts when we handle this in
>> zero_one_operation. Unlike what was done earlier, we now change the stmt
>> (with propagate_op_to_signle use or by directly) such that the value
>> computed by stmt is no longer what it used to be. Because of this, what is
>> computed in undistribute_ops_list and rewrite_expr_tree are also changed.
>>
>> undistribute_ops_list already expects this but rewrite_expr_tree will not if
>> we dont pass the changed as an argument.
>>
>> The way I am fixing this now is, in linearize_expr_tree, I set ops_changed
>> to true if we change NEGATE_EXPR to (-1) MULT_EXPR (OP). Then when we call
>> zero_one_operation with ops_changed = true, I replace all the LHS in
>> zero_one_operation with the new SSA and replace all the uses. I also call
>> the rewrite_expr_tree with changed = false in this case.
>>
>> Does this make sense? Bootstrapped and regression tested for
>> x86_64-linux-gnu without any new regressions.
>
> I don't think this solves the issue.  zero_one_operation associates the
> chain starting at the first *def and it will change the intermediate values
> of _all_ of the stmts visited until the operation to be removed is found.
> Note that this is independent of whether try_special_add_to_ops did anything.
>
> Even for the regular undistribution cases we get this wrong.
>
> So we need to back-track in zero_one_operation, replacing each LHS
> and in the end the op in the opvector of the main chain.  That's basically
> the same as if we'd do a regular re-assoc operation on the sub-chains.
> Take their subops, simulate zero_one_operation by
> appending the cancelling operation and optimizing the oplist, and then
> materializing the associated ops via rewrite_expr_tree.
>
Here is a draft patch which records the stmt chain when in
zero_one_operation and then fixes it when OP is removed. when we
update *def, that will update the ops vector. Does this looks sane?


Bootstrapped and regression tested on x86_64-linux-gnu with no new regressions.

Thanks,
Kugan

[-- Attachment #2: p.txt --]
[-- Type: text/plain, Size: 5127 bytes --]

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c b/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
index e69de29..049eddc 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
@@ -0,0 +1,36 @@
+/* PR tree-optimization/72835.  */
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+struct struct_1 {
+    unsigned int m1 : 6 ;
+    unsigned int m2 : 24 ;
+    unsigned int m3 : 6 ;
+};
+
+unsigned short var_32 = 0x2d10;
+
+struct struct_1 s1;
+
+void init ()
+{
+  s1.m1 = 4;
+  s1.m2 = 0x7ca4b8;
+  s1.m3 = 24;
+}
+
+void foo ()
+{
+  unsigned int c
+    = ((unsigned int) s1.m2) * (-((unsigned int) s1.m3))
+    + (var_32) * (-((unsigned int) (s1.m1)));
+  if (c != 4098873984)
+    __builtin_abort ();
+}
+
+int main ()
+{
+    init ();
+    foo ();
+    return 0;
+}
diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
index 7fd7550..c7f6a66 100644
--- a/gcc/tree-ssa-reassoc.c
+++ b/gcc/tree-ssa-reassoc.c
@@ -1148,6 +1148,49 @@ decrement_power (gimple *stmt)
     }
 }
 
+/* Replace SSA defined by STMT and replace all its uses with new
+   SSA.  Also return the new SSA.  */
+
+static tree
+make_new_ssa_for_def (gimple *stmt)
+{
+  gimple *use_stmt;
+  use_operand_p use;
+  imm_use_iterator iter;
+  tree new_lhs;
+  tree lhs = gimple_assign_lhs (stmt);
+  gcc_assert (has_single_use (lhs));
+
+  new_lhs = make_ssa_name (TREE_TYPE (lhs));
+  gimple_set_lhs (stmt, new_lhs);
+
+  /* Also need to update GIMPLE_DEBUGs.  */
+  FOR_EACH_IMM_USE_STMT (use_stmt, iter, lhs)
+    {
+      FOR_EACH_IMM_USE_ON_STMT (use, iter)
+	SET_USE (use, new_lhs);
+      update_stmt (use_stmt);
+    }
+  return new_lhs;
+}
+
+/* Replace all SSAs defined in STMTS_TO_FIX and replace its
+   uses with new SSAs.  Also do this for the stmt that defines DEF
+   if *DEF is not OP.  */
+
+static void
+make_new_ssa_for_all_defs (tree *def, tree op,
+			   auto_vec<gimple *> &stmts_to_fix)
+{
+  unsigned i;
+  gimple *stmt = SSA_NAME_DEF_STMT (*def);
+  if (*def != op
+      && gimple_code (stmt) != GIMPLE_NOP)
+    *def = make_new_ssa_for_def (stmt);
+  FOR_EACH_VEC_ELT (stmts_to_fix, i, stmt)
+    make_new_ssa_for_def (stmt);
+}
+
 /* Find the single immediate use of STMT's LHS, and replace it
    with OP.  Remove STMT.  If STMT's LHS is the same as *DEF,
    replace *DEF with OP as well.  */
@@ -1186,6 +1229,9 @@ static void
 zero_one_operation (tree *def, enum tree_code opcode, tree op)
 {
   gimple *stmt = SSA_NAME_DEF_STMT (*def);
+  /* PR72835 - Record the stmt chain that has to be updated such that
+     we dont use the same LHS when the values computed are different.  */
+  auto_vec<gimple *> stmts_to_fix;
 
   do
     {
@@ -1195,6 +1241,7 @@ zero_one_operation (tree *def, enum tree_code opcode, tree op)
 	{
 	  if (stmt_is_power_of_op (stmt, op))
 	    {
+	      make_new_ssa_for_all_defs (def, op, stmts_to_fix);
 	      if (decrement_power (stmt) == 1)
 		propagate_op_to_single_use (op, stmt, def);
 	      return;
@@ -1204,6 +1251,7 @@ zero_one_operation (tree *def, enum tree_code opcode, tree op)
 	      if (gimple_assign_rhs1 (stmt) == op)
 		{
 		  tree cst = build_minus_one_cst (TREE_TYPE (op));
+		  make_new_ssa_for_all_defs (def, op, stmts_to_fix);
 		  propagate_op_to_single_use (cst, stmt, def);
 		  return;
 		}
@@ -1212,6 +1260,7 @@ zero_one_operation (tree *def, enum tree_code opcode, tree op)
 		{
 		  gimple_assign_set_rhs_code
 		    (stmt, TREE_CODE (gimple_assign_rhs1 (stmt)));
+		  make_new_ssa_for_all_defs (def, op, stmts_to_fix);
 		  return;
 		}
 	    }
@@ -1228,6 +1277,7 @@ zero_one_operation (tree *def, enum tree_code opcode, tree op)
 	{
 	  if (name == op)
 	    name = gimple_assign_rhs2 (stmt);
+	  make_new_ssa_for_all_defs (def, op, stmts_to_fix);
 	  propagate_op_to_single_use (name, stmt, def);
 	  return;
 	}
@@ -1243,6 +1293,8 @@ zero_one_operation (tree *def, enum tree_code opcode, tree op)
 	  gimple *stmt2 = SSA_NAME_DEF_STMT (gimple_assign_rhs2 (stmt));
 	  if (stmt_is_power_of_op (stmt2, op))
 	    {
+	      stmts_to_fix.safe_push (stmt2);
+	      make_new_ssa_for_all_defs (def, op, stmts_to_fix);
 	      if (decrement_power (stmt2) == 1)
 		propagate_op_to_single_use (op, stmt2, def);
 	      return;
@@ -1253,14 +1305,18 @@ zero_one_operation (tree *def, enum tree_code opcode, tree op)
 	      if (gimple_assign_rhs1 (stmt2) == op)
 		{
 		  tree cst = build_minus_one_cst (TREE_TYPE (op));
+		  stmts_to_fix.safe_push (stmt2);
+		  make_new_ssa_for_all_defs (def, op, stmts_to_fix);
 		  propagate_op_to_single_use (cst, stmt2, def);
 		  return;
 		}
 	      else if (integer_minus_onep (op)
 		       || real_minus_onep (op))
 		{
+		  stmts_to_fix.safe_push (stmt2);
 		  gimple_assign_set_rhs_code
 		    (stmt2, TREE_CODE (gimple_assign_rhs1 (stmt2)));
+		  make_new_ssa_for_all_defs (def, op, stmts_to_fix);
 		  return;
 		}
 	    }
@@ -1270,6 +1326,7 @@ zero_one_operation (tree *def, enum tree_code opcode, tree op)
       gcc_assert (name != op
 		  && TREE_CODE (name) == SSA_NAME);
       stmt = SSA_NAME_DEF_STMT (name);
+      stmts_to_fix.safe_push (stmt);
     }
   while (1);
 }

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[Richard Biener]

On Fri, Sep 2, 2016 at 10:09 AM, Kugan Vivekanandarajah
<kugan.vivekanandarajah@linaro.org> wrote:
> Hi Richard,
>
> On 25 August 2016 at 22:24, Richard Biener <richard.guenther@gmail.com> wrote:
>> On Thu, Aug 11, 2016 at 1:09 AM, kugan
>> <kugan.vivekanandarajah@linaro.org> wrote:
>>> Hi,
>>>
>>>
>>> On 10/08/16 20:28, Richard Biener wrote:
>>>>
>>>> On Wed, Aug 10, 2016 at 10:57 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>
>>>>> On Wed, Aug 10, 2016 at 08:51:32AM +1000, kugan wrote:
>>>>>>
>>>>>> I see it now. The problem is we are just looking at (-1) being in the
>>>>>> ops
>>>>>> list for passing changed to rewrite_expr_tree in the case of
>>>>>> multiplication
>>>>>> by negate.  If we have combined (-1), as in the testcase, we will not
>>>>>> have
>>>>>> the (-1) and will pass changed=false to rewrite_expr_tree.
>>>>>>
>>>>>> We should set changed based on what happens in try_special_add_to_ops.
>>>>>> Attached patch does this. Bootstrap and regression testing are ongoing.
>>>>>> Is
>>>>>> this OK for trunk if there is no regression.
>>>>>
>>>>>
>>>>> I think the bug is elsewhere.  In particular in
>>>>> undistribute_ops_list/zero_one_operation/decrement_power.
>>>>> All those look problematic in this regard, they change RHS of statements
>>>>> to something that holds a different value, while keeping the LHS.
>>>>> So, generally you should instead just add a new stmt next to the old one,
>>>>> and adjust data structures (replace the old SSA_NAME in some ->op with
>>>>> the new one).  decrement_power might be a problem here, dunno if all the
>>>>> builtins are const in all cases that DSE would kill the old one,
>>>>> Richard, any preferences for that?  reset flow sensitive info + reset
>>>>> debug
>>>>> stmt uses, or something different?  Though, replacing the LHS with a new
>>>>> anonymous SSA_NAME might be needed too, in case it is before SSA_NAME of
>>>>> a
>>>>> user var that doesn't yet have any debug stmts.
>>>>
>>>>
>>>> I'd say replacing the LHS is the way to go, with calling the appropriate
>>>> helper
>>>> on the old stmt to generate a debug stmt for it / its uses (would need
>>>> to look it
>>>> up here).
>>>>
>>>
>>> Here is an attempt to fix it. The problem arises when in
>>> undistribute_ops_list, we linearize_expr_tree such that NEGATE_EXPR is added
>>> (-1) MULT_EXPR (OP). Real problem starts when we handle this in
>>> zero_one_operation. Unlike what was done earlier, we now change the stmt
>>> (with propagate_op_to_signle use or by directly) such that the value
>>> computed by stmt is no longer what it used to be. Because of this, what is
>>> computed in undistribute_ops_list and rewrite_expr_tree are also changed.
>>>
>>> undistribute_ops_list already expects this but rewrite_expr_tree will not if
>>> we dont pass the changed as an argument.
>>>
>>> The way I am fixing this now is, in linearize_expr_tree, I set ops_changed
>>> to true if we change NEGATE_EXPR to (-1) MULT_EXPR (OP). Then when we call
>>> zero_one_operation with ops_changed = true, I replace all the LHS in
>>> zero_one_operation with the new SSA and replace all the uses. I also call
>>> the rewrite_expr_tree with changed = false in this case.
>>>
>>> Does this make sense? Bootstrapped and regression tested for
>>> x86_64-linux-gnu without any new regressions.
>>
>> I don't think this solves the issue.  zero_one_operation associates the
>> chain starting at the first *def and it will change the intermediate values
>> of _all_ of the stmts visited until the operation to be removed is found.
>> Note that this is independent of whether try_special_add_to_ops did anything.
>>
>> Even for the regular undistribution cases we get this wrong.
>>
>> So we need to back-track in zero_one_operation, replacing each LHS
>> and in the end the op in the opvector of the main chain.  That's basically
>> the same as if we'd do a regular re-assoc operation on the sub-chains.
>> Take their subops, simulate zero_one_operation by
>> appending the cancelling operation and optimizing the oplist, and then
>> materializing the associated ops via rewrite_expr_tree.
>>
> Here is a draft patch which records the stmt chain when in
> zero_one_operation and then fixes it when OP is removed. when we
> update *def, that will update the ops vector. Does this looks sane?

Yes.  A few comments below

+  /* PR72835 - Record the stmt chain that has to be updated such that
+     we dont use the same LHS when the values computed are different.  */
+  auto_vec<gimple *> stmts_to_fix;

use auto_vec<gimple *, 64> here so we get stack allocation only most
of the times

          if (stmt_is_power_of_op (stmt, op))
            {
+             make_new_ssa_for_all_defs (def, op, stmts_to_fix);
              if (decrement_power (stmt) == 1)
                propagate_op_to_single_use (op, stmt, def);

for the cases you end up with propagate_op_to_single_use its argument
stmt is handled superfluosly in the new SSA making, I suggest to pop it
from the stmts_to_fix vector in that case.  I suggest to break; instead
of return in all cases and do the make_new_ssa_for_all_defs call at
the function end instead.

@@ -1253,14 +1305,18 @@ zero_one_operation (tree *def, enum tree_code
opcode, tree op)
              if (gimple_assign_rhs1 (stmt2) == op)
                {
                  tree cst = build_minus_one_cst (TREE_TYPE (op));
+                 stmts_to_fix.safe_push (stmt2);
+                 make_new_ssa_for_all_defs (def, op, stmts_to_fix);
                  propagate_op_to_single_use (cst, stmt2, def);
                  return;

this safe_push should be unnecessary for the above reason (others are
conditionally unnecessary).

I thought about simplifying the whole thing by instead of clearing an
op from the chain pre-pend
one that does the job by means of visiting the chain from reassoc
itself but that doesn't work out
for RDIV_EXPR nor does it play well with undistribute handling
mutliple opportunities on the same
chain.

Thanks,
Richard.


>
> Bootstrapped and regression tested on x86_64-linux-gnu with no new regressions.
>
> Thanks,
> Kugan

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[kugan]

[-- Attachment #1: Type: text/plain, Size: 6464 bytes --]

Hi Richard,

On 14/09/16 21:31, Richard Biener wrote:
> On Fri, Sep 2, 2016 at 10:09 AM, Kugan Vivekanandarajah
> <kugan.vivekanandarajah@linaro.org> wrote:
>> Hi Richard,
>>
>> On 25 August 2016 at 22:24, Richard Biener <richard.guenther@gmail.com> wrote:
>>> On Thu, Aug 11, 2016 at 1:09 AM, kugan
>>> <kugan.vivekanandarajah@linaro.org> wrote:
>>>> Hi,
>>>>
>>>>
>>>> On 10/08/16 20:28, Richard Biener wrote:
>>>>>
>>>>> On Wed, Aug 10, 2016 at 10:57 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>>
>>>>>> On Wed, Aug 10, 2016 at 08:51:32AM +1000, kugan wrote:
>>>>>>>
>>>>>>> I see it now. The problem is we are just looking at (-1) being in the
>>>>>>> ops
>>>>>>> list for passing changed to rewrite_expr_tree in the case of
>>>>>>> multiplication
>>>>>>> by negate.  If we have combined (-1), as in the testcase, we will not
>>>>>>> have
>>>>>>> the (-1) and will pass changed=false to rewrite_expr_tree.
>>>>>>>
>>>>>>> We should set changed based on what happens in try_special_add_to_ops.
>>>>>>> Attached patch does this. Bootstrap and regression testing are ongoing.
>>>>>>> Is
>>>>>>> this OK for trunk if there is no regression.
>>>>>>
>>>>>>
>>>>>> I think the bug is elsewhere.  In particular in
>>>>>> undistribute_ops_list/zero_one_operation/decrement_power.
>>>>>> All those look problematic in this regard, they change RHS of statements
>>>>>> to something that holds a different value, while keeping the LHS.
>>>>>> So, generally you should instead just add a new stmt next to the old one,
>>>>>> and adjust data structures (replace the old SSA_NAME in some ->op with
>>>>>> the new one).  decrement_power might be a problem here, dunno if all the
>>>>>> builtins are const in all cases that DSE would kill the old one,
>>>>>> Richard, any preferences for that?  reset flow sensitive info + reset
>>>>>> debug
>>>>>> stmt uses, or something different?  Though, replacing the LHS with a new
>>>>>> anonymous SSA_NAME might be needed too, in case it is before SSA_NAME of
>>>>>> a
>>>>>> user var that doesn't yet have any debug stmts.
>>>>>
>>>>>
>>>>> I'd say replacing the LHS is the way to go, with calling the appropriate
>>>>> helper
>>>>> on the old stmt to generate a debug stmt for it / its uses (would need
>>>>> to look it
>>>>> up here).
>>>>>
>>>>
>>>> Here is an attempt to fix it. The problem arises when in
>>>> undistribute_ops_list, we linearize_expr_tree such that NEGATE_EXPR is added
>>>> (-1) MULT_EXPR (OP). Real problem starts when we handle this in
>>>> zero_one_operation. Unlike what was done earlier, we now change the stmt
>>>> (with propagate_op_to_signle use or by directly) such that the value
>>>> computed by stmt is no longer what it used to be. Because of this, what is
>>>> computed in undistribute_ops_list and rewrite_expr_tree are also changed.
>>>>
>>>> undistribute_ops_list already expects this but rewrite_expr_tree will not if
>>>> we dont pass the changed as an argument.
>>>>
>>>> The way I am fixing this now is, in linearize_expr_tree, I set ops_changed
>>>> to true if we change NEGATE_EXPR to (-1) MULT_EXPR (OP). Then when we call
>>>> zero_one_operation with ops_changed = true, I replace all the LHS in
>>>> zero_one_operation with the new SSA and replace all the uses. I also call
>>>> the rewrite_expr_tree with changed = false in this case.
>>>>
>>>> Does this make sense? Bootstrapped and regression tested for
>>>> x86_64-linux-gnu without any new regressions.
>>>
>>> I don't think this solves the issue.  zero_one_operation associates the
>>> chain starting at the first *def and it will change the intermediate values
>>> of _all_ of the stmts visited until the operation to be removed is found.
>>> Note that this is independent of whether try_special_add_to_ops did anything.
>>>
>>> Even for the regular undistribution cases we get this wrong.
>>>
>>> So we need to back-track in zero_one_operation, replacing each LHS
>>> and in the end the op in the opvector of the main chain.  That's basically
>>> the same as if we'd do a regular re-assoc operation on the sub-chains.
>>> Take their subops, simulate zero_one_operation by
>>> appending the cancelling operation and optimizing the oplist, and then
>>> materializing the associated ops via rewrite_expr_tree.
>>>
>> Here is a draft patch which records the stmt chain when in
>> zero_one_operation and then fixes it when OP is removed. when we
>> update *def, that will update the ops vector. Does this looks sane?
>
> Yes.  A few comments below
>
> +  /* PR72835 - Record the stmt chain that has to be updated such that
> +     we dont use the same LHS when the values computed are different.  */
> +  auto_vec<gimple *> stmts_to_fix;
>
> use auto_vec<gimple *, 64> here so we get stack allocation only most
> of the times
Done.

>           if (stmt_is_power_of_op (stmt, op))
>             {
> +             make_new_ssa_for_all_defs (def, op, stmts_to_fix);
>               if (decrement_power (stmt) == 1)
>                 propagate_op_to_single_use (op, stmt, def);
>
> for the cases you end up with propagate_op_to_single_use its argument
> stmt is handled superfluosly in the new SSA making, I suggest to pop it
> from the stmts_to_fix vector in that case.  I suggest to break; instead
> of return in all cases and do the make_new_ssa_for_all_defs call at
> the function end instead.
>
Done.

> @@ -1253,14 +1305,18 @@ zero_one_operation (tree *def, enum tree_code
> opcode, tree op)
>               if (gimple_assign_rhs1 (stmt2) == op)
>                 {
>                   tree cst = build_minus_one_cst (TREE_TYPE (op));
> +                 stmts_to_fix.safe_push (stmt2);
> +                 make_new_ssa_for_all_defs (def, op, stmts_to_fix);
>                   propagate_op_to_single_use (cst, stmt2, def);
>                   return;
>
> this safe_push should be unnecessary for the above reason (others are
> conditionally unnecessary).
>
Done.

Bootstrapped and regression tested on X86_64-linux-gnu with no new 
regression. Is this OK?

Thanks,
Kugan

> I thought about simplifying the whole thing by instead of clearing an
> op from the chain pre-pend
> one that does the job by means of visiting the chain from reassoc
> itself but that doesn't work out
> for RDIV_EXPR nor does it play well with undistribute handling
> mutliple opportunities on the same
> chain.
>
> Thanks,
> Richard.
>
>
>>
>> Bootstrapped and regression tested on x86_64-linux-gnu with no new regressions.
>>
>> Thanks,
>> Kugan

[-- Attachment #2: p.txt --]
[-- Type: text/plain, Size: 5286 bytes --]

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c b/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
index e69de29..468e0f0 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
@@ -0,0 +1,37 @@
+/* PR tree-optimization/72835.  */
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+/* { dg-require-effective-target int32plus } */
+
+struct struct_1 {
+    unsigned int m1 : 6 ;
+    unsigned int m2 : 24 ;
+    unsigned int m3 : 6 ;
+};
+
+unsigned short var_32 = 0x2d10;
+
+struct struct_1 s1;
+
+void init ()
+{
+  s1.m1 = 4;
+  s1.m2 = 0x7ca4b8;
+  s1.m3 = 24;
+}
+
+void foo ()
+{
+  unsigned int c
+    = ((unsigned int) s1.m2) * (-((unsigned int) s1.m3))
+    + (var_32) * (-((unsigned int) (s1.m1)));
+  if (c != 4098873984)
+    __builtin_abort ();
+}
+
+int main ()
+{
+    init ();
+    foo ();
+    return 0;
+}
diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
index 7fd7550..24e9dd6 100644
--- a/gcc/tree-ssa-reassoc.c
+++ b/gcc/tree-ssa-reassoc.c
@@ -1148,6 +1148,52 @@ decrement_power (gimple *stmt)
     }
 }
 
+/* Replace SSA defined by STMT and replace all its uses with new
+   SSA.  Also return the new SSA.  */
+
+static tree
+make_new_ssa_for_def (gimple *stmt)
+{
+  gimple *use_stmt;
+  use_operand_p use;
+  imm_use_iterator iter;
+  tree new_lhs;
+  tree lhs = gimple_assign_lhs (stmt);
+
+  new_lhs = make_ssa_name (TREE_TYPE (lhs));
+  gimple_set_lhs (stmt, new_lhs);
+
+  /* Also need to update GIMPLE_DEBUGs.  */
+  FOR_EACH_IMM_USE_STMT (use_stmt, iter, lhs)
+    {
+      FOR_EACH_IMM_USE_ON_STMT (use, iter)
+	SET_USE (use, new_lhs);
+      update_stmt (use_stmt);
+    }
+  return new_lhs;
+}
+
+/* Replace all SSAs defined in STMTS_TO_FIX and replace its
+   uses with new SSAs.  Also do this for the stmt that defines DEF
+   if *DEF is not OP.  */
+
+static void
+make_new_ssa_for_all_defs (tree *def, tree op,
+			   auto_vec<gimple *, 64> &stmts_to_fix)
+{
+  unsigned i;
+  gimple *stmt;
+
+  if (*def != op
+      && TREE_CODE (*def) == SSA_NAME
+      && (stmt = SSA_NAME_DEF_STMT (*def))
+      && gimple_code (stmt) != GIMPLE_NOP)
+    *def = make_new_ssa_for_def (stmt);
+
+  FOR_EACH_VEC_ELT (stmts_to_fix, i, stmt)
+    make_new_ssa_for_def (stmt);
+}
+
 /* Find the single immediate use of STMT's LHS, and replace it
    with OP.  Remove STMT.  If STMT's LHS is the same as *DEF,
    replace *DEF with OP as well.  */
@@ -1186,6 +1232,9 @@ static void
 zero_one_operation (tree *def, enum tree_code opcode, tree op)
 {
   gimple *stmt = SSA_NAME_DEF_STMT (*def);
+  /* PR72835 - Record the stmt chain that has to be updated such that
+     we dont use the same LHS when the values computed are different.  */
+  auto_vec<gimple *, 64> stmts_to_fix;
 
   do
     {
@@ -1196,23 +1245,29 @@ zero_one_operation (tree *def, enum tree_code opcode, tree op)
 	  if (stmt_is_power_of_op (stmt, op))
 	    {
 	      if (decrement_power (stmt) == 1)
-		propagate_op_to_single_use (op, stmt, def);
-	      return;
+		{
+		  if (stmts_to_fix.length () > 0)
+		    stmts_to_fix.pop ();
+		  propagate_op_to_single_use (op, stmt, def);
+		}
+	      break;
 	    }
 	  else if (gimple_assign_rhs_code (stmt) == NEGATE_EXPR)
 	    {
 	      if (gimple_assign_rhs1 (stmt) == op)
 		{
 		  tree cst = build_minus_one_cst (TREE_TYPE (op));
+		  if (stmts_to_fix.length () > 0)
+		    stmts_to_fix.pop ();
 		  propagate_op_to_single_use (cst, stmt, def);
-		  return;
+		  break;
 		}
 	      else if (integer_minus_onep (op)
 		       || real_minus_onep (op))
 		{
 		  gimple_assign_set_rhs_code
 		    (stmt, TREE_CODE (gimple_assign_rhs1 (stmt)));
-		  return;
+		  break;
 		}
 	    }
 	}
@@ -1228,8 +1283,10 @@ zero_one_operation (tree *def, enum tree_code opcode, tree op)
 	{
 	  if (name == op)
 	    name = gimple_assign_rhs2 (stmt);
+	  if (stmts_to_fix.length () > 0)
+	    stmts_to_fix.pop ();
 	  propagate_op_to_single_use (name, stmt, def);
-	  return;
+	  break;
 	}
 
       /* We might have a multiply of two __builtin_pow* calls, and
@@ -1245,7 +1302,9 @@ zero_one_operation (tree *def, enum tree_code opcode, tree op)
 	    {
 	      if (decrement_power (stmt2) == 1)
 		propagate_op_to_single_use (op, stmt2, def);
-	      return;
+	      else
+		stmts_to_fix.safe_push (stmt2);
+	      break;
 	    }
 	  else if (is_gimple_assign (stmt2)
 		   && gimple_assign_rhs_code (stmt2) == NEGATE_EXPR)
@@ -1254,14 +1313,15 @@ zero_one_operation (tree *def, enum tree_code opcode, tree op)
 		{
 		  tree cst = build_minus_one_cst (TREE_TYPE (op));
 		  propagate_op_to_single_use (cst, stmt2, def);
-		  return;
+		  break;
 		}
 	      else if (integer_minus_onep (op)
 		       || real_minus_onep (op))
 		{
+		  stmts_to_fix.safe_push (stmt2);
 		  gimple_assign_set_rhs_code
 		    (stmt2, TREE_CODE (gimple_assign_rhs1 (stmt2)));
-		  return;
+		  break;
 		}
 	    }
 	}
@@ -1270,8 +1330,12 @@ zero_one_operation (tree *def, enum tree_code opcode, tree op)
       gcc_assert (name != op
 		  && TREE_CODE (name) == SSA_NAME);
       stmt = SSA_NAME_DEF_STMT (name);
+      stmts_to_fix.safe_push (stmt);
     }
   while (1);
+
+  if (stmts_to_fix.length () > 0)
+    make_new_ssa_for_all_defs (def, op, stmts_to_fix);
 }
 
 /* Returns true if statement S1 dominates statement S2.  Like

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[Richard Biener]

On Sun, Sep 18, 2016 at 10:21 PM, kugan
<kugan.vivekanandarajah@linaro.org> wrote:
> Hi Richard,
>
>
> On 14/09/16 21:31, Richard Biener wrote:
>>
>> On Fri, Sep 2, 2016 at 10:09 AM, Kugan Vivekanandarajah
>> <kugan.vivekanandarajah@linaro.org> wrote:
>>>
>>> Hi Richard,
>>>
>>> On 25 August 2016 at 22:24, Richard Biener <richard.guenther@gmail.com>
>>> wrote:
>>>>
>>>> On Thu, Aug 11, 2016 at 1:09 AM, kugan
>>>> <kugan.vivekanandarajah@linaro.org> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>>
>>>>> On 10/08/16 20:28, Richard Biener wrote:
>>>>>>
>>>>>>
>>>>>> On Wed, Aug 10, 2016 at 10:57 AM, Jakub Jelinek <jakub@redhat.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Wed, Aug 10, 2016 at 08:51:32AM +1000, kugan wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> I see it now. The problem is we are just looking at (-1) being in
>>>>>>>> the
>>>>>>>> ops
>>>>>>>> list for passing changed to rewrite_expr_tree in the case of
>>>>>>>> multiplication
>>>>>>>> by negate.  If we have combined (-1), as in the testcase, we will
>>>>>>>> not
>>>>>>>> have
>>>>>>>> the (-1) and will pass changed=false to rewrite_expr_tree.
>>>>>>>>
>>>>>>>> We should set changed based on what happens in
>>>>>>>> try_special_add_to_ops.
>>>>>>>> Attached patch does this. Bootstrap and regression testing are
>>>>>>>> ongoing.
>>>>>>>> Is
>>>>>>>> this OK for trunk if there is no regression.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I think the bug is elsewhere.  In particular in
>>>>>>> undistribute_ops_list/zero_one_operation/decrement_power.
>>>>>>> All those look problematic in this regard, they change RHS of
>>>>>>> statements
>>>>>>> to something that holds a different value, while keeping the LHS.
>>>>>>> So, generally you should instead just add a new stmt next to the old
>>>>>>> one,
>>>>>>> and adjust data structures (replace the old SSA_NAME in some ->op
>>>>>>> with
>>>>>>> the new one).  decrement_power might be a problem here, dunno if all
>>>>>>> the
>>>>>>> builtins are const in all cases that DSE would kill the old one,
>>>>>>> Richard, any preferences for that?  reset flow sensitive info + reset
>>>>>>> debug
>>>>>>> stmt uses, or something different?  Though, replacing the LHS with a
>>>>>>> new
>>>>>>> anonymous SSA_NAME might be needed too, in case it is before SSA_NAME
>>>>>>> of
>>>>>>> a
>>>>>>> user var that doesn't yet have any debug stmts.
>>>>>>
>>>>>>
>>>>>>
>>>>>> I'd say replacing the LHS is the way to go, with calling the
>>>>>> appropriate
>>>>>> helper
>>>>>> on the old stmt to generate a debug stmt for it / its uses (would need
>>>>>> to look it
>>>>>> up here).
>>>>>>
>>>>>
>>>>> Here is an attempt to fix it. The problem arises when in
>>>>> undistribute_ops_list, we linearize_expr_tree such that NEGATE_EXPR is
>>>>> added
>>>>> (-1) MULT_EXPR (OP). Real problem starts when we handle this in
>>>>> zero_one_operation. Unlike what was done earlier, we now change the
>>>>> stmt
>>>>> (with propagate_op_to_signle use or by directly) such that the value
>>>>> computed by stmt is no longer what it used to be. Because of this, what
>>>>> is
>>>>> computed in undistribute_ops_list and rewrite_expr_tree are also
>>>>> changed.
>>>>>
>>>>> undistribute_ops_list already expects this but rewrite_expr_tree will
>>>>> not if
>>>>> we dont pass the changed as an argument.
>>>>>
>>>>> The way I am fixing this now is, in linearize_expr_tree, I set
>>>>> ops_changed
>>>>> to true if we change NEGATE_EXPR to (-1) MULT_EXPR (OP). Then when we
>>>>> call
>>>>> zero_one_operation with ops_changed = true, I replace all the LHS in
>>>>> zero_one_operation with the new SSA and replace all the uses. I also
>>>>> call
>>>>> the rewrite_expr_tree with changed = false in this case.
>>>>>
>>>>> Does this make sense? Bootstrapped and regression tested for
>>>>> x86_64-linux-gnu without any new regressions.
>>>>
>>>>
>>>> I don't think this solves the issue.  zero_one_operation associates the
>>>> chain starting at the first *def and it will change the intermediate
>>>> values
>>>> of _all_ of the stmts visited until the operation to be removed is
>>>> found.
>>>> Note that this is independent of whether try_special_add_to_ops did
>>>> anything.
>>>>
>>>> Even for the regular undistribution cases we get this wrong.
>>>>
>>>> So we need to back-track in zero_one_operation, replacing each LHS
>>>> and in the end the op in the opvector of the main chain.  That's
>>>> basically
>>>> the same as if we'd do a regular re-assoc operation on the sub-chains.
>>>> Take their subops, simulate zero_one_operation by
>>>> appending the cancelling operation and optimizing the oplist, and then
>>>> materializing the associated ops via rewrite_expr_tree.
>>>>
>>> Here is a draft patch which records the stmt chain when in
>>> zero_one_operation and then fixes it when OP is removed. when we
>>> update *def, that will update the ops vector. Does this looks sane?
>>
>>
>> Yes.  A few comments below
>>
>> +  /* PR72835 - Record the stmt chain that has to be updated such that
>> +     we dont use the same LHS when the values computed are different.  */
>> +  auto_vec<gimple *> stmts_to_fix;
>>
>> use auto_vec<gimple *, 64> here so we get stack allocation only most
>> of the times
>
> Done.
>
>>           if (stmt_is_power_of_op (stmt, op))
>>             {
>> +             make_new_ssa_for_all_defs (def, op, stmts_to_fix);
>>               if (decrement_power (stmt) == 1)
>>                 propagate_op_to_single_use (op, stmt, def);
>>
>> for the cases you end up with propagate_op_to_single_use its argument
>> stmt is handled superfluosly in the new SSA making, I suggest to pop it
>> from the stmts_to_fix vector in that case.  I suggest to break; instead
>> of return in all cases and do the make_new_ssa_for_all_defs call at
>> the function end instead.
>>
> Done.
>
>> @@ -1253,14 +1305,18 @@ zero_one_operation (tree *def, enum tree_code
>> opcode, tree op)
>>               if (gimple_assign_rhs1 (stmt2) == op)
>>                 {
>>                   tree cst = build_minus_one_cst (TREE_TYPE (op));
>> +                 stmts_to_fix.safe_push (stmt2);
>> +                 make_new_ssa_for_all_defs (def, op, stmts_to_fix);
>>                   propagate_op_to_single_use (cst, stmt2, def);
>>                   return;
>>
>> this safe_push should be unnecessary for the above reason (others are
>> conditionally unnecessary).
>>
> Done.
>
> Bootstrapped and regression tested on X86_64-linux-gnu with no new
> regression. Is this OK?

+static void
+make_new_ssa_for_all_defs (tree *def, tree op,
+               auto_vec<gimple *, 64> &stmts_to_fix)

I think you need to use vec<gimple *> &stmts_to_fix here AFAIK.

Ok with that change.

Richard.

> Thanks,
> Kugan
>
>
>> I thought about simplifying the whole thing by instead of clearing an
>> op from the chain pre-pend
>> one that does the job by means of visiting the chain from reassoc
>> itself but that doesn't work out
>> for RDIV_EXPR nor does it play well with undistribute handling
>> mutliple opportunities on the same
>> chain.
>>
>> Thanks,
>> Richard.
>>
>>
>>>
>>> Bootstrapped and regression tested on x86_64-linux-gnu with no new
>>> regressions.
>>>
>>> Thanks,
>>> Kugan

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[kugan]

Hi Richard,
Thanks for the review.

On 19/09/16 23:40, Richard Biener wrote:
> On Sun, Sep 18, 2016 at 10:21 PM, kugan
> <kugan.vivekanandarajah@linaro.org> wrote:
>> Hi Richard,
>>
>>
>> On 14/09/16 21:31, Richard Biener wrote:
>>>
>>> On Fri, Sep 2, 2016 at 10:09 AM, Kugan Vivekanandarajah
>>> <kugan.vivekanandarajah@linaro.org> wrote:
>>>>
>>>> Hi Richard,
>>>>
>>>> On 25 August 2016 at 22:24, Richard Biener <richard.guenther@gmail.com>
>>>> wrote:
>>>>>
>>>>> On Thu, Aug 11, 2016 at 1:09 AM, kugan
>>>>> <kugan.vivekanandarajah@linaro.org> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>>
>>>>>> On 10/08/16 20:28, Richard Biener wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Wed, Aug 10, 2016 at 10:57 AM, Jakub Jelinek <jakub@redhat.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On Wed, Aug 10, 2016 at 08:51:32AM +1000, kugan wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I see it now. The problem is we are just looking at (-1) being in
>>>>>>>>> the
>>>>>>>>> ops
>>>>>>>>> list for passing changed to rewrite_expr_tree in the case of
>>>>>>>>> multiplication
>>>>>>>>> by negate.  If we have combined (-1), as in the testcase, we will
>>>>>>>>> not
>>>>>>>>> have
>>>>>>>>> the (-1) and will pass changed=false to rewrite_expr_tree.
>>>>>>>>>
>>>>>>>>> We should set changed based on what happens in
>>>>>>>>> try_special_add_to_ops.
>>>>>>>>> Attached patch does this. Bootstrap and regression testing are
>>>>>>>>> ongoing.
>>>>>>>>> Is
>>>>>>>>> this OK for trunk if there is no regression.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I think the bug is elsewhere.  In particular in
>>>>>>>> undistribute_ops_list/zero_one_operation/decrement_power.
>>>>>>>> All those look problematic in this regard, they change RHS of
>>>>>>>> statements
>>>>>>>> to something that holds a different value, while keeping the LHS.
>>>>>>>> So, generally you should instead just add a new stmt next to the old
>>>>>>>> one,
>>>>>>>> and adjust data structures (replace the old SSA_NAME in some ->op
>>>>>>>> with
>>>>>>>> the new one).  decrement_power might be a problem here, dunno if all
>>>>>>>> the
>>>>>>>> builtins are const in all cases that DSE would kill the old one,
>>>>>>>> Richard, any preferences for that?  reset flow sensitive info + reset
>>>>>>>> debug
>>>>>>>> stmt uses, or something different?  Though, replacing the LHS with a
>>>>>>>> new
>>>>>>>> anonymous SSA_NAME might be needed too, in case it is before SSA_NAME
>>>>>>>> of
>>>>>>>> a
>>>>>>>> user var that doesn't yet have any debug stmts.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I'd say replacing the LHS is the way to go, with calling the
>>>>>>> appropriate
>>>>>>> helper
>>>>>>> on the old stmt to generate a debug stmt for it / its uses (would need
>>>>>>> to look it
>>>>>>> up here).
>>>>>>>
>>>>>>
>>>>>> Here is an attempt to fix it. The problem arises when in
>>>>>> undistribute_ops_list, we linearize_expr_tree such that NEGATE_EXPR is
>>>>>> added
>>>>>> (-1) MULT_EXPR (OP). Real problem starts when we handle this in
>>>>>> zero_one_operation. Unlike what was done earlier, we now change the
>>>>>> stmt
>>>>>> (with propagate_op_to_signle use or by directly) such that the value
>>>>>> computed by stmt is no longer what it used to be. Because of this, what
>>>>>> is
>>>>>> computed in undistribute_ops_list and rewrite_expr_tree are also
>>>>>> changed.
>>>>>>
>>>>>> undistribute_ops_list already expects this but rewrite_expr_tree will
>>>>>> not if
>>>>>> we dont pass the changed as an argument.
>>>>>>
>>>>>> The way I am fixing this now is, in linearize_expr_tree, I set
>>>>>> ops_changed
>>>>>> to true if we change NEGATE_EXPR to (-1) MULT_EXPR (OP). Then when we
>>>>>> call
>>>>>> zero_one_operation with ops_changed = true, I replace all the LHS in
>>>>>> zero_one_operation with the new SSA and replace all the uses. I also
>>>>>> call
>>>>>> the rewrite_expr_tree with changed = false in this case.
>>>>>>
>>>>>> Does this make sense? Bootstrapped and regression tested for
>>>>>> x86_64-linux-gnu without any new regressions.
>>>>>
>>>>>
>>>>> I don't think this solves the issue.  zero_one_operation associates the
>>>>> chain starting at the first *def and it will change the intermediate
>>>>> values
>>>>> of _all_ of the stmts visited until the operation to be removed is
>>>>> found.
>>>>> Note that this is independent of whether try_special_add_to_ops did
>>>>> anything.
>>>>>
>>>>> Even for the regular undistribution cases we get this wrong.
>>>>>
>>>>> So we need to back-track in zero_one_operation, replacing each LHS
>>>>> and in the end the op in the opvector of the main chain.  That's
>>>>> basically
>>>>> the same as if we'd do a regular re-assoc operation on the sub-chains.
>>>>> Take their subops, simulate zero_one_operation by
>>>>> appending the cancelling operation and optimizing the oplist, and then
>>>>> materializing the associated ops via rewrite_expr_tree.
>>>>>
>>>> Here is a draft patch which records the stmt chain when in
>>>> zero_one_operation and then fixes it when OP is removed. when we
>>>> update *def, that will update the ops vector. Does this looks sane?
>>>
>>>
>>> Yes.  A few comments below
>>>
>>> +  /* PR72835 - Record the stmt chain that has to be updated such that
>>> +     we dont use the same LHS when the values computed are different.  */
>>> +  auto_vec<gimple *> stmts_to_fix;
>>>
>>> use auto_vec<gimple *, 64> here so we get stack allocation only most
>>> of the times
>>
>> Done.
>>
>>>           if (stmt_is_power_of_op (stmt, op))
>>>             {
>>> +             make_new_ssa_for_all_defs (def, op, stmts_to_fix);
>>>               if (decrement_power (stmt) == 1)
>>>                 propagate_op_to_single_use (op, stmt, def);
>>>
>>> for the cases you end up with propagate_op_to_single_use its argument
>>> stmt is handled superfluosly in the new SSA making, I suggest to pop it
>>> from the stmts_to_fix vector in that case.  I suggest to break; instead
>>> of return in all cases and do the make_new_ssa_for_all_defs call at
>>> the function end instead.
>>>
>> Done.
>>
>>> @@ -1253,14 +1305,18 @@ zero_one_operation (tree *def, enum tree_code
>>> opcode, tree op)
>>>               if (gimple_assign_rhs1 (stmt2) == op)
>>>                 {
>>>                   tree cst = build_minus_one_cst (TREE_TYPE (op));
>>> +                 stmts_to_fix.safe_push (stmt2);
>>> +                 make_new_ssa_for_all_defs (def, op, stmts_to_fix);
>>>                   propagate_op_to_single_use (cst, stmt2, def);
>>>                   return;
>>>
>>> this safe_push should be unnecessary for the above reason (others are
>>> conditionally unnecessary).
>>>
>> Done.
>>
>> Bootstrapped and regression tested on X86_64-linux-gnu with no new
>> regression. Is this OK?
>
> +static void
> +make_new_ssa_for_all_defs (tree *def, tree op,
> +               auto_vec<gimple *, 64> &stmts_to_fix)
>
> I think you need to use vec<gimple *> &stmts_to_fix here AFAIK.
>

This is what I had. With that I get:
error: invalid initialization of reference of type ‘auto_vec<gimple*>&’ 
from expression of type ‘auto_vec<gimple*, 64ul>

Is this a bug?

Thanks,
Kugan

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[Richard Biener]

On Tue, Sep 20, 2016 at 1:32 AM, kugan
<kugan.vivekanandarajah@linaro.org> wrote:
> Hi Richard,
> Thanks for the review.
>
>
> On 19/09/16 23:40, Richard Biener wrote:
>>
>> On Sun, Sep 18, 2016 at 10:21 PM, kugan
>> <kugan.vivekanandarajah@linaro.org> wrote:
>>>
>>> Hi Richard,
>>>
>>>
>>> On 14/09/16 21:31, Richard Biener wrote:
>>>>
>>>>
>>>> On Fri, Sep 2, 2016 at 10:09 AM, Kugan Vivekanandarajah
>>>> <kugan.vivekanandarajah@linaro.org> wrote:
>>>>>
>>>>>
>>>>> Hi Richard,
>>>>>
>>>>> On 25 August 2016 at 22:24, Richard Biener <richard.guenther@gmail.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> On Thu, Aug 11, 2016 at 1:09 AM, kugan
>>>>>> <kugan.vivekanandarajah@linaro.org> wrote:
>>>>>>>
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>>
>>>>>>> On 10/08/16 20:28, Richard Biener wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Wed, Aug 10, 2016 at 10:57 AM, Jakub Jelinek <jakub@redhat.com>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Wed, Aug 10, 2016 at 08:51:32AM +1000, kugan wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I see it now. The problem is we are just looking at (-1) being in
>>>>>>>>>> the
>>>>>>>>>> ops
>>>>>>>>>> list for passing changed to rewrite_expr_tree in the case of
>>>>>>>>>> multiplication
>>>>>>>>>> by negate.  If we have combined (-1), as in the testcase, we will
>>>>>>>>>> not
>>>>>>>>>> have
>>>>>>>>>> the (-1) and will pass changed=false to rewrite_expr_tree.
>>>>>>>>>>
>>>>>>>>>> We should set changed based on what happens in
>>>>>>>>>> try_special_add_to_ops.
>>>>>>>>>> Attached patch does this. Bootstrap and regression testing are
>>>>>>>>>> ongoing.
>>>>>>>>>> Is
>>>>>>>>>> this OK for trunk if there is no regression.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I think the bug is elsewhere.  In particular in
>>>>>>>>> undistribute_ops_list/zero_one_operation/decrement_power.
>>>>>>>>> All those look problematic in this regard, they change RHS of
>>>>>>>>> statements
>>>>>>>>> to something that holds a different value, while keeping the LHS.
>>>>>>>>> So, generally you should instead just add a new stmt next to the
>>>>>>>>> old
>>>>>>>>> one,
>>>>>>>>> and adjust data structures (replace the old SSA_NAME in some ->op
>>>>>>>>> with
>>>>>>>>> the new one).  decrement_power might be a problem here, dunno if
>>>>>>>>> all
>>>>>>>>> the
>>>>>>>>> builtins are const in all cases that DSE would kill the old one,
>>>>>>>>> Richard, any preferences for that?  reset flow sensitive info +
>>>>>>>>> reset
>>>>>>>>> debug
>>>>>>>>> stmt uses, or something different?  Though, replacing the LHS with
>>>>>>>>> a
>>>>>>>>> new
>>>>>>>>> anonymous SSA_NAME might be needed too, in case it is before
>>>>>>>>> SSA_NAME
>>>>>>>>> of
>>>>>>>>> a
>>>>>>>>> user var that doesn't yet have any debug stmts.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I'd say replacing the LHS is the way to go, with calling the
>>>>>>>> appropriate
>>>>>>>> helper
>>>>>>>> on the old stmt to generate a debug stmt for it / its uses (would
>>>>>>>> need
>>>>>>>> to look it
>>>>>>>> up here).
>>>>>>>>
>>>>>>>
>>>>>>> Here is an attempt to fix it. The problem arises when in
>>>>>>> undistribute_ops_list, we linearize_expr_tree such that NEGATE_EXPR
>>>>>>> is
>>>>>>> added
>>>>>>> (-1) MULT_EXPR (OP). Real problem starts when we handle this in
>>>>>>> zero_one_operation. Unlike what was done earlier, we now change the
>>>>>>> stmt
>>>>>>> (with propagate_op_to_signle use or by directly) such that the value
>>>>>>> computed by stmt is no longer what it used to be. Because of this,
>>>>>>> what
>>>>>>> is
>>>>>>> computed in undistribute_ops_list and rewrite_expr_tree are also
>>>>>>> changed.
>>>>>>>
>>>>>>> undistribute_ops_list already expects this but rewrite_expr_tree will
>>>>>>> not if
>>>>>>> we dont pass the changed as an argument.
>>>>>>>
>>>>>>> The way I am fixing this now is, in linearize_expr_tree, I set
>>>>>>> ops_changed
>>>>>>> to true if we change NEGATE_EXPR to (-1) MULT_EXPR (OP). Then when we
>>>>>>> call
>>>>>>> zero_one_operation with ops_changed = true, I replace all the LHS in
>>>>>>> zero_one_operation with the new SSA and replace all the uses. I also
>>>>>>> call
>>>>>>> the rewrite_expr_tree with changed = false in this case.
>>>>>>>
>>>>>>> Does this make sense? Bootstrapped and regression tested for
>>>>>>> x86_64-linux-gnu without any new regressions.
>>>>>>
>>>>>>
>>>>>>
>>>>>> I don't think this solves the issue.  zero_one_operation associates
>>>>>> the
>>>>>> chain starting at the first *def and it will change the intermediate
>>>>>> values
>>>>>> of _all_ of the stmts visited until the operation to be removed is
>>>>>> found.
>>>>>> Note that this is independent of whether try_special_add_to_ops did
>>>>>> anything.
>>>>>>
>>>>>> Even for the regular undistribution cases we get this wrong.
>>>>>>
>>>>>> So we need to back-track in zero_one_operation, replacing each LHS
>>>>>> and in the end the op in the opvector of the main chain.  That's
>>>>>> basically
>>>>>> the same as if we'd do a regular re-assoc operation on the sub-chains.
>>>>>> Take their subops, simulate zero_one_operation by
>>>>>> appending the cancelling operation and optimizing the oplist, and then
>>>>>> materializing the associated ops via rewrite_expr_tree.
>>>>>>
>>>>> Here is a draft patch which records the stmt chain when in
>>>>> zero_one_operation and then fixes it when OP is removed. when we
>>>>> update *def, that will update the ops vector. Does this looks sane?
>>>>
>>>>
>>>>
>>>> Yes.  A few comments below
>>>>
>>>> +  /* PR72835 - Record the stmt chain that has to be updated such that
>>>> +     we dont use the same LHS when the values computed are different.
>>>> */
>>>> +  auto_vec<gimple *> stmts_to_fix;
>>>>
>>>> use auto_vec<gimple *, 64> here so we get stack allocation only most
>>>> of the times
>>>
>>>
>>> Done.
>>>
>>>>           if (stmt_is_power_of_op (stmt, op))
>>>>             {
>>>> +             make_new_ssa_for_all_defs (def, op, stmts_to_fix);
>>>>               if (decrement_power (stmt) == 1)
>>>>                 propagate_op_to_single_use (op, stmt, def);
>>>>
>>>> for the cases you end up with propagate_op_to_single_use its argument
>>>> stmt is handled superfluosly in the new SSA making, I suggest to pop it
>>>> from the stmts_to_fix vector in that case.  I suggest to break; instead
>>>> of return in all cases and do the make_new_ssa_for_all_defs call at
>>>> the function end instead.
>>>>
>>> Done.
>>>
>>>> @@ -1253,14 +1305,18 @@ zero_one_operation (tree *def, enum tree_code
>>>> opcode, tree op)
>>>>               if (gimple_assign_rhs1 (stmt2) == op)
>>>>                 {
>>>>                   tree cst = build_minus_one_cst (TREE_TYPE (op));
>>>> +                 stmts_to_fix.safe_push (stmt2);
>>>> +                 make_new_ssa_for_all_defs (def, op, stmts_to_fix);
>>>>                   propagate_op_to_single_use (cst, stmt2, def);
>>>>                   return;
>>>>
>>>> this safe_push should be unnecessary for the above reason (others are
>>>> conditionally unnecessary).
>>>>
>>> Done.
>>>
>>> Bootstrapped and regression tested on X86_64-linux-gnu with no new
>>> regression. Is this OK?
>>
>>
>> +static void
>> +make_new_ssa_for_all_defs (tree *def, tree op,
>> +               auto_vec<gimple *, 64> &stmts_to_fix)
>>
>> I think you need to use vec<gimple *> &stmts_to_fix here AFAIK.
>>
>
> This is what I had. With that I get:
> error: invalid initialization of reference of type ‘auto_vec<gimple*>&’ from
> expression of type ‘auto_vec<gimple*, 64ul>
>
> Is this a bug?

You need to use vec<gimple *>, not auto_vec<gimple *>.

Richard.

> Thanks,
> Kugan

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[Andrew Pinski]

On Tue, Aug 9, 2016 at 2:42 PM, kugan <kugan.vivekanandarajah@linaro.org> wrote:
>
>
> On 09/08/16 23:43, kugan wrote:
>>
>> Hi,
>>
>> The test-case in PR72835 is failing with -O2 and passing with
>> -fno-tree-reassoc. It also passes with -O2 -fno-tree-vrp.
>>
>> diff of .115t.dse2 and .116t.reassoc1 for the c++ testcase is as
>> follows, which looks OK.
>>
>> +  unsigned int _16;
>> +  unsigned int _17;
>> +  unsigned int _18;
>>
>>     <bb 2>:
>>     _1 = s1.m2;
>>     _2 = (unsigned int) _1;
>>     _3 = s1.m3;
>>     _4 = (unsigned int) _3;
>> -  _5 = -_4;
>> -  _6 = _2 * _5;
>> +  _5 = _4;
>> +  _6 = _5 * _2;
>>     var_32.0_7 = var_32;
>>     _8 = (unsigned int) var_32.0_7;
>>     _9 = s1.m1;
>>     _10 = (unsigned int) _9;
>> -  _11 = -_10;
>> -  _12 = _8 * _11;
>> -  c_14 = _6 + _12;
>> +  _11 = _10;
>> +  _12 = _11 * _8;
>> +  _16 = _12 + _6;
>> +  _18 = _16;
>> +  _17 = -_18;
>> +  c_14 = _17;
>>     if (c_14 != 4098873984)
>>
>>
>> However, I noticed that when we re-associate and assign different
>> operands to the stmts, we are not resetting the flow information to the
>> LHS. This looks wrong. Attached patch resets it. With this, the
>> testcases in TH PR is now working.
>>
>>
>> Bootstrap and regression testing for x86_64-linux-gnu is in progress. Is
>> this OK for trunk if there is no regression.
>
>
> There was no new regression while testing. I also moved the testcase from
> gcc.dg/torture/pr72835.c to gcc.dg/tree-ssa/pr72835.c. Is this OK for trunk?


Why did you move the test-case from gcc.dg/torture to gcc.dg/tree-ssa?
 I think most executable testcases (that was using standard options)
should be in tortue testcases to do a full sweep of the options.

Thanks,
Andrew

>
> Thanks,
> Kugan
>
> gcc/testsuite/ChangeLog:
>
> 2016-08-10  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>         PR tree-optimization/72835
>         * gcc.dg/tree-ssa/pr72835.c: New test.
>
> gcc/ChangeLog:
>
> 2016-08-10  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>
>         PR tree-optimization/72835
>         * tree-ssa-reassoc.c (rewrite_expr_tree): Reset value_range of LHS
> when
>         operands are changed.
>         (rewrite_expr_tree_parallel): Likewise.

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[kugan]

Hi Andrew,

On 10/08/16 07:50, Andrew Pinski wrote:
> On Tue, Aug 9, 2016 at 2:42 PM, kugan <kugan.vivekanandarajah@linaro.org> wrote:
>>
>>
>> On 09/08/16 23:43, kugan wrote:
>>>
>>> Hi,
>>>
>>> The test-case in PR72835 is failing with -O2 and passing with
>>> -fno-tree-reassoc. It also passes with -O2 -fno-tree-vrp.
>>>
>>> diff of .115t.dse2 and .116t.reassoc1 for the c++ testcase is as
>>> follows, which looks OK.
>>>
>>> +  unsigned int _16;
>>> +  unsigned int _17;
>>> +  unsigned int _18;
>>>
>>>     <bb 2>:
>>>     _1 = s1.m2;
>>>     _2 = (unsigned int) _1;
>>>     _3 = s1.m3;
>>>     _4 = (unsigned int) _3;
>>> -  _5 = -_4;
>>> -  _6 = _2 * _5;
>>> +  _5 = _4;
>>> +  _6 = _5 * _2;
>>>     var_32.0_7 = var_32;
>>>     _8 = (unsigned int) var_32.0_7;
>>>     _9 = s1.m1;
>>>     _10 = (unsigned int) _9;
>>> -  _11 = -_10;
>>> -  _12 = _8 * _11;
>>> -  c_14 = _6 + _12;
>>> +  _11 = _10;
>>> +  _12 = _11 * _8;
>>> +  _16 = _12 + _6;
>>> +  _18 = _16;
>>> +  _17 = -_18;
>>> +  c_14 = _17;
>>>     if (c_14 != 4098873984)
>>>
>>>
>>> However, I noticed that when we re-associate and assign different
>>> operands to the stmts, we are not resetting the flow information to the
>>> LHS. This looks wrong. Attached patch resets it. With this, the
>>> testcases in TH PR is now working.
>>>
>>>
>>> Bootstrap and regression testing for x86_64-linux-gnu is in progress. Is
>>> this OK for trunk if there is no regression.
>>
>>
>> There was no new regression while testing. I also moved the testcase from
>> gcc.dg/torture/pr72835.c to gcc.dg/tree-ssa/pr72835.c. Is this OK for trunk?
>
>
> Why did you move the test-case from gcc.dg/torture to gcc.dg/tree-ssa?
>  I think most executable testcases (that was using standard options)
> should be in tortue testcases to do a full sweep of the options.

I thought It was unnecessary to run with all the options as -O2 would 
trigger this. I am OK with moving it to gcc.dg/torture/pr72835.c if you 
prefer.

Thanks,
Kugan

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[Georg-Johann Lay]

On 09.08.2016 15:43, kugan wrote:
> Hi,
>
> The test-case in PR72835 is failing with -O2 and passing with
> -fno-tree-reassoc. It also passes with -O2 -fno-tree-vrp.
>
> diff of .115t.dse2 and .116t.reassoc1 for the c++ testcase is as follows, which
> looks OK.
>
> +  unsigned int _16;
> +  unsigned int _17;
> +  unsigned int _18;
>
>    <bb 2>:
>    _1 = s1.m2;
>    _2 = (unsigned int) _1;
>    _3 = s1.m3;
>    _4 = (unsigned int) _3;
> -  _5 = -_4;
> -  _6 = _2 * _5;
> +  _5 = _4;
> +  _6 = _5 * _2;
>    var_32.0_7 = var_32;
>    _8 = (unsigned int) var_32.0_7;
>    _9 = s1.m1;
>    _10 = (unsigned int) _9;
> -  _11 = -_10;
> -  _12 = _8 * _11;
> -  c_14 = _6 + _12;
> +  _11 = _10;
> +  _12 = _11 * _8;
> +  _16 = _12 + _6;
> +  _18 = _16;
> +  _17 = -_18;
> +  c_14 = _17;
>    if (c_14 != 4098873984)
>
>
> However, I noticed that when we re-associate and assign different operands to
> the stmts, we are not resetting the flow information to the LHS. This looks
> wrong. Attached patch resets it. With this, the testcases in TH PR is now working.
>
>
> Bootstrap and regression testing for x86_64-linux-gnu is in progress. Is this
> OK for trunk if there is no regression.
>
> Thanks,
> Kugan
>
> gcc/testsuite/ChangeLog:
>
> 2016-08-09  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>     PR tree-optimization/72835
>     * gcc.dg/torture/pr72835.c: New test.
>
> gcc/ChangeLog:
>
> 2016-08-09  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>     PR tree-optimization/72835
>     * tree-ssa-reassoc.c (rewrite_expr_tree): Reset value_range of LHS when
>     operands are changed.
>     (rewrite_expr_tree_parallel): Likewise.
>
> Hi,
>
> The test-case in PR72835 is failing with -O2 and passing with -fno-tree-reassoc. It also passes with -O2 -fno-tree-vrp.
>
> diff of .115t.dse2 and .116t.reassoc1 for the c++ testcase is as follows, which looks OK.
>
> +  unsigned int _16;
> +  unsigned int _17;
> +  unsigned int _18;
>
>    <bb 2>:
>    _1 = s1.m2;
>    _2 = (unsigned int) _1;
>    _3 = s1.m3;
>    _4 = (unsigned int) _3;
> -  _5 = -_4;
> -  _6 = _2 * _5;
> +  _5 = _4;
> +  _6 = _5 * _2;
>    var_32.0_7 = var_32;
>    _8 = (unsigned int) var_32.0_7;
>    _9 = s1.m1;
>    _10 = (unsigned int) _9;
> -  _11 = -_10;
> -  _12 = _8 * _11;
> -  c_14 = _6 + _12;
> +  _11 = _10;
> +  _12 = _11 * _8;
> +  _16 = _12 + _6;
> +  _18 = _16;
> +  _17 = -_18;
> +  c_14 = _17;
>    if (c_14 != 4098873984)
>
>
> However, I noticed that when we re-associate and assign different operands to the stmts, we are not resetting the flow information to the LHS. This looks wrong. Attached patch resets it. With this, the testcases in TH PR is now working.
>
>
> Bootstrap and regression testing for x86_64-linux-gnu is in progress. Is this OK for trunk if there is no regression.
>
> Thanks,
> Kugan
>
> gcc/testsuite/ChangeLog:
>
> 2016-08-09  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>     PR tree-optimization/72835
>     * gcc.dg/torture/pr72835.c: New test.
>
> gcc/ChangeLog:
>
> 2016-08-09  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>     PR tree-optimization/72835
>     * tree-ssa-reassoc.c (rewrite_expr_tree): Reset value_range of LHS when
>     operands are changed.
>     (rewrite_expr_tree_parallel): Likewise.
>
> pr72835.txt
>
> diff --git a/gcc/testsuite/gcc.dg/torture/pr72835.c b/gcc/testsuite/gcc.dg/torture/pr72835.c
> index e69de29..d7d0a8d 100644
> --- a/gcc/testsuite/gcc.dg/torture/pr72835.c
> +++ b/gcc/testsuite/gcc.dg/torture/pr72835.c
> @@ -0,0 +1,36 @@
> +/* PR tree-optimization/72835 */
> +/* { dg-do run } */
> +/* { dg-options "-O2" } */
> +
> +struct struct_1 {
> +    unsigned int m1 : 6 ;
> +    unsigned int m2 : 24 ;

This test needs dg-require effective target int32plus because it will break on 
int=16bit targets.

> +    unsigned int m3 : 6 ;
> +};
> +
> +unsigned short var_32 = 0x2d10;
> +
> +struct struct_1 s1;
> +
> +void init ()
> +{
> +  s1.m1 = 4;
> +  s1.m2 = 0x7ca4b8;
> +  s1.m3 = 24;
> +}
> +
> +void foo ()
> +{
> +  unsigned int c =
> +    ((unsigned int) s1.m2) * (-((unsigned int) s1.m3))
> +    + (var_32) * (-((unsigned int) (s1.m1)));
> +  if (c != 4098873984)
> +    __builtin_abort ();
> +}
> +
> +int main ()
> +{
> +    init ();
> +    foo ();
> +    return 0;
> +}