Bug 72835 - [7 Regression] Incorrect arithmetic optimization involving bitfield arguments
Summary: [7 Regression] Incorrect arithmetic optimization involving bitfield arguments
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 7.0
: P3 normal
Target Milestone: 7.0
Assignee: kugan
URL:
Keywords: wrong-code
Depends on:
Blocks: yarpgen
  Show dependency treegraph
 
Reported: 2016-08-08 13:29 UTC by Dmitry Babokin
Modified: 2021-11-01 23:07 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Known to work: 6.1.0
Known to fail:
Last reconfirmed: 2016-08-09 00:00:00


Attachments
reproducer (255 bytes, text/x-csrc)
2016-08-08 13:29 UTC, Dmitry Babokin
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Babokin 2016-08-08 13:29:02 UTC
Created attachment 39074 [details]
reproducer

-O0 and -O2 produce different results, while the test doesn't contain undefined behavior.

> g++ -O0 bit_field_math.cpp ; ./a.out
4098873984
> g++ -O2 bit_field_math.cpp ; ./a.out
0
> g++ --version
gcc (Revision=238966/svn-rev:238969/) 7.0.0 20160801 (experimental)

GCC version 6 works correctly.

The test case is attached.

One thing that might look suspicious in the test is negation of unsigned int. But it's well defined in the standard (same bit pattern as if it's signed, but the result is treated as unsigned).
Comment 1 Martin Liška 2016-08-09 10:39:40 UTC
Confirmed, it's caused by tree-reassoc pass:

$ g++ pr72835.cpp -O2 -fno-tree-reassoc ; ./a.out
4098873984
Comment 2 Richard Biener 2016-08-09 11:21:46 UTC
C testcase:

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 =
   // 0x7ca4b8 * -(24)
   ((unsigned int) s1.m2) * (-((unsigned int) s1.m3))
   +
   // 0x2d10 * -(4) = 0xb440
   (var_32) * (-((unsigned int) (s1.m1)));

  if (c != 4098873984)
    __builtin_abort ();
}

int main () {
    init ();
    foo ();
    return 0;
}
Comment 3 kugan 2016-08-09 12:29:10 UTC
Looking into it. 

diff of .115t.dse2 and .116t.reassoc1 is for the c++ testcase:

+  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)


Also works with -fno-tree-vrp
Comment 4 kugan 2016-08-09 13:14:54 UTC
Looks like it was a latent issue. In rewrite_expr_tree, when re-associate operands, we should reset range_info for the LHS. We don’t do that now. Following patch fixes the test case. 


diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
index 7fd7550..6272d98 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)


I think we also need to do the same in rewrite_expr_tree_parallel.
Comment 5 kugan 2016-09-21 03:28:57 UTC
Author: kugan
Date: Wed Sep 21 03:28:24 2016
New Revision: 240299

URL: https://gcc.gnu.org/viewcvs?rev=240299&root=gcc&view=rev
Log:
Incorrect arithmetic optimization involving bitfield arguments

gcc/ChangeLog:

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

	PR tree-optimization/72835
	* tree-ssa-reassoc.c (make_new_ssa_for_def): New.
	(make_new_ssa_for_all_defs): Likewise.
	(zero_one_operation): Replace all SSA_NAMEs defined in the chain.


gcc/testsuite/ChangeLog:

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

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


Added:
    trunk/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-reassoc.c
Comment 6 kugan 2016-11-21 22:49:59 UTC
Fixed in trunk.