| Message ID | 98613cff-7c48-1a56-0014-6d87c35a8f26@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 >
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
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 {