Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[Richard Biener]

On Tue, Sep 20, 2016 at 1:32 AM, kugan
 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
>>  wrote:
>>>
>>> Hi Richard,
>>>
>>>
>>> On 14/09/16 21:31, Richard Biener wrote:


 On Fri, Sep 2, 2016 at 10:09 AM, Kugan Vivekanandarajah
  wrote:
>
>
> Hi Richard,
>
> On 25 August 2016 at 22:24, Richard Biener 
> wrote:
>>
>>
>> On Thu, Aug 11, 2016 at 1:09 AM, kugan
>>  wrote:
>>>
>>>
>>> Hi,
>>>
>>>
>>> On 10/08/16 20:28, Richard Biener wrote:



 On Wed, Aug 10, 2016 at 10:57 AM, 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.




 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 

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
 wrote:

Hi Richard,


On 14/09/16 21:31, Richard Biener wrote:


On Fri, Sep 2, 2016 at 10:09 AM, Kugan Vivekanandarajah
 wrote:


Hi Richard,

On 25 August 2016 at 22:24, Richard Biener 
wrote:


On Thu, Aug 11, 2016 at 1:09 AM, kugan
 wrote:


Hi,


On 10/08/16 20:28, Richard Biener wrote:



On Wed, Aug 10, 2016 at 10:57 AM, 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.




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 stmts_to_fix;

use auto_vec 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.


@@ 

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[Richard Biener]

On Sun, Sep 18, 2016 at 10:21 PM, kugan
 wrote:
> Hi Richard,
>
>
> On 14/09/16 21:31, Richard Biener wrote:
>>
>> On Fri, Sep 2, 2016 at 10:09 AM, Kugan Vivekanandarajah
>>  wrote:
>>>
>>> Hi Richard,
>>>
>>> On 25 August 2016 at 22:24, Richard Biener 
>>> wrote:

 On Thu, Aug 11, 2016 at 1:09 AM, kugan
  wrote:
>
> Hi,
>
>
> On 10/08/16 20:28, Richard Biener wrote:
>>
>>
>> On Wed, Aug 10, 2016 at 10:57 AM, 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.
>>
>>
>>
>> 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 

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[kugan]

Hi Richard,

On 14/09/16 21:31, Richard Biener wrote:

On Fri, Sep 2, 2016 at 10:09 AM, Kugan Vivekanandarajah
 wrote:

Hi Richard,

On 25 August 2016 at 22:24, Richard Biener  wrote:

On Thu, Aug 11, 2016 at 1:09 AM, kugan
 wrote:

Hi,


On 10/08/16 20:28, Richard Biener wrote:


On Wed, Aug 10, 2016 at 10:57 AM, 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.



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 stmts_to_fix;

use auto_vec 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 

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;

   :
   _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  

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

gcc/ChangeLog:

2016-08-09  Kugan Vivekanandarajah  

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;

   :
   _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  

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

gcc/ChangeLog:

2016-08-09  Kugan Vivekanandarajah  

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;
+}

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[Richard Biener]

On Fri, Sep 2, 2016 at 10:09 AM, Kugan Vivekanandarajah
 wrote:
> Hi Richard,
>
> On 25 August 2016 at 22:24, Richard Biener  wrote:
>> On Thu, Aug 11, 2016 at 1:09 AM, kugan
>>  wrote:
>>> Hi,
>>>
>>>
>>> On 10/08/16 20:28, Richard Biener wrote:

 On Wed, Aug 10, 2016 at 10:57 AM, 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.


 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 stmts_to_fix;

use auto_vec 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.  

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[Kugan Vivekanandarajah]

Hi Richard,

On 25 August 2016 at 22:24, Richard Biener  wrote:
> On Thu, Aug 11, 2016 at 1:09 AM, kugan
>  wrote:
>> Hi,
>>
>>
>> On 10/08/16 20:28, Richard Biener wrote:
>>>
>>> On Wed, Aug 10, 2016 at 10:57 AM, 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.
>>>
>>>
>>> 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
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 ();
+   

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[Richard Biener]

On Thu, Aug 11, 2016 at 1:09 AM, kugan
 wrote:
> Hi,
>
>
> On 10/08/16 20:28, Richard Biener wrote:
>>
>> On Wed, Aug 10, 2016 at 10:57 AM, 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.
>>
>>
>> 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  
>
> PR tree-optimization/72835
> * gcc.dg/tree-ssa/pr72835.c: New test.
>
> gcc/ChangeLog:
>
> 2016-08-10  Kugan Vivekanandarajah  
>
> 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]

Ping?

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

Thanks,
Kugan

On 11 August 2016 at 09:09, kugan  wrote:
> Hi,
>
>
> On 10/08/16 20:28, Richard Biener wrote:
>>
>> On Wed, Aug 10, 2016 at 10:57 AM, 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.
>>
>>
>> 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  
>
> PR tree-optimization/72835
> * gcc.dg/tree-ssa/pr72835.c: New test.
>
> gcc/ChangeLog:
>
> 2016-08-10  Kugan Vivekanandarajah  
>
> 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]

Hi,

On 10/08/16 20:28, Richard Biener wrote:

On Wed, Aug 10, 2016 at 10:57 AM, 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.


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  

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

gcc/ChangeLog:

2016-08-10  Kugan Vivekanandarajah  

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
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 *, 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)

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[Richard Biener]

On Wed, Aug 10, 2016 at 10:57 AM, 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.

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]

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
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 *, 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, _p, _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 *ops, struct loop *loop)
+  vec *ops, struct loop *loop,
+

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]

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

[kugan]

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  

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

gcc/ChangeLog:

2016-08-10  Kugan Vivekanandarajah  

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.


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 *, gimple *,
+static bool linearize_expr_tree (vec *, 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 *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 *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 *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 *ops, gimple *stmt,
 bool is_associative, bool set_visited)
 {
@@ -4512,6 +4518,7 @@ linearize_expr_tree (vec *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 @@ 

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]

Hi Andrew,

On 10/08/16 07:50, Andrew Pinski wrote:

On Tue, Aug 9, 2016 at 2:42 PM, kugan  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;

:
_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

[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  

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

gcc/ChangeLog:

2016-08-10  Kugan Vivekanandarajah  

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

[Andrew Pinski]

On Tue, Aug 9, 2016 at 2:42 PM, kugan  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;
>>
>> :
>> _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  
>
> PR tree-optimization/72835
> * gcc.dg/tree-ssa/pr72835.c: New test.
>
> gcc/ChangeLog:
>
> 2016-08-10  Kugan Vivekanandarajah  
>
>
> 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

[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  
> 
>   PR tree-optimization/72835
>   * gcc.dg/tree-ssa/pr72835.c: New test.
> 
> gcc/ChangeLog:
> 
> 2016-08-10  Kugan Vivekanandarajah  
> 
>   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]

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;

:
_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  

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

gcc/ChangeLog:

2016-08-10  Kugan Vivekanandarajah  

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.
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
{

[PR72835] Incorrect arithmetic optimization involving bitfield arguments

[kugan]

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;

   :
   _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  

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

gcc/ChangeLog:

2016-08-09  Kugan Vivekanandarajah  

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.
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
{