diff mbox

[PR72835] Incorrect arithmetic optimization involving bitfield arguments

Message ID 0c53b0f3-4af6-387c-9350-95b1ae85850d@linaro.org
State New
Headers show

Commit Message

Kugan Vivekanandarajah Aug. 9, 2016, 10:51 p.m. UTC 
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.

Comments

Kugan Vivekanandarajah Aug. 10, 2016, 1:45 a.m. UTC | #1 
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
Jakub Jelinek Aug. 10, 2016, 8:57 a.m. UTC | #2 
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
Richard Biener Aug. 10, 2016, 10:28 a.m. UTC | #3 
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
diff mbox

Patch

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