| Message ID | 20151203205838.GI5675@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 3 Dec 2015, Jakub Jelinek wrote: > Hi! > > As mentioned in the PR, maybe_optimize_range_tests considers basic blocks > with not just the final GIMPLE_COND (or for last_bb store feeding into PHI), > but also assign stmts that don't trap, don't have side-effects and where > the SSA_NAMEs they set are used only in their own bb. > Now, if we decide to optimize some range test, we can change some conditions > on previous bbs and that means we could execute some basic blocks that > wouldn't be executed in the original program. As the stmts don't set > anything used in other bbs, they are most likely dead after the > optimization, but the problem on the testcase is that because of the > condition changes in previous bb we end up with incorrect value range > for some SSA_NAME(s). That can result in the miscompilation of the testcase > on certain targets. > > Fixed by resetting the value range info of such SSA_NAMEs. I believe it > shouldn't be a big deal, they will be mostly dead anyway. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2015-12-03 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/68671 > * tree-ssa-reassoc.c (maybe_optimize_range_tests): For basic > blocks starting with the successor of first bb we've modified > and ending with last_bb, reset value ranges of all integral > SSA_NAMEs set in those basic blocks. > > * gcc.dg/pr68671.c: New test. > > --- gcc/tree-ssa-reassoc.c.jj 2015-11-18 11:22:51.000000000 +0100 > +++ gcc/tree-ssa-reassoc.c 2015-12-03 18:12:08.915210122 +0100 > @@ -3204,7 +3204,7 @@ maybe_optimize_range_tests (gimple *stmt > any_changes = optimize_range_tests (ERROR_MARK, &ops); > if (any_changes) > { > - unsigned int idx; > + unsigned int idx, max_idx = 0; > /* update_ops relies on has_single_use predicates returning the > same values as it did during get_ops earlier. Additionally it > never removes statements, only adds new ones and it should walk > @@ -3220,6 +3220,7 @@ maybe_optimize_range_tests (gimple *stmt > { > tree new_op; > > + max_idx = idx; > stmt = last_stmt (bb); > new_op = update_ops (bbinfo[idx].op, > (enum tree_code) > @@ -3289,6 +3290,10 @@ maybe_optimize_range_tests (gimple *stmt > && ops[bbinfo[idx].first_idx]->op != NULL_TREE) > { > gcond *cond_stmt = as_a <gcond *> (last_stmt (bb)); > + > + if (idx > max_idx) > + max_idx = idx; > + > if (integer_zerop (ops[bbinfo[idx].first_idx]->op)) > gimple_cond_make_false (cond_stmt); > else if (integer_onep (ops[bbinfo[idx].first_idx]->op)) > @@ -3305,6 +3310,30 @@ maybe_optimize_range_tests (gimple *stmt > if (bb == first_bb) > break; > } > + > + /* The above changes could result in basic blocks after the first > + modified one, up to and including last_bb, to be executed even if > + they would not be in the original program. If the value ranges of > + assignment lhs' in those bbs were dependent on the conditions > + guarding those basic blocks which now can change, the VRs might > + be incorrect. As no_side_effect_bb should ensure those SSA_NAMEs > + are only used within the same bb, it should be not a big deal if > + we just reset all the VRs in those bbs. See PR68671. */ > + for (bb = last_bb, idx = 0; idx < max_idx; bb = single_pred (bb), idx++) > + { > + gimple_stmt_iterator gsi; > + for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi)) > + { > + gimple *g = gsi_stmt (gsi); > + if (!is_gimple_assign (g)) > + continue; > + tree lhs = gimple_assign_lhs (g); > + if (TREE_CODE (lhs) != SSA_NAME) > + continue; > + if (INTEGRAL_TYPE_P (TREE_TYPE (lhs))) > + SSA_NAME_RANGE_INFO (lhs) = NULL; Please use reset_flow_sensitive_info (lhs); Ok with that change. Thanks, Richard. > + } > + } > } > } > > --- gcc/testsuite/gcc.dg/pr68671.c.jj 2015-12-03 18:19:24.769104484 +0100 > +++ gcc/testsuite/gcc.dg/pr68671.c 2015-12-03 18:19:07.000000000 +0100 > @@ -0,0 +1,23 @@ > +/* PR tree-optimization/68671 */ > +/* { dg-do run } */ > +/* { dg-options " -O2 -fno-tree-dce" } */ > + > +volatile int a = -1; > +volatile int b; > + > +static inline int > +fn1 (signed char p1, int p2) > +{ > + return (p1 < 0) || (p1 > (1 >> p2)) ? 0 : (p1 << 1); > +} > + > +int > +main () > +{ > + signed char c = a; > + b = fn1 (c, 1); > + c = ((128 | c) < 0 ? 1 : 0); > + if (c != 1) > + __builtin_abort (); > + return 0; > +} > > Jakub > >
On Fri, Dec 04, 2015 at 09:15:25AM +0100, Richard Biener wrote: > > + modified one, up to and including last_bb, to be executed even if > > + they would not be in the original program. If the value ranges of > > + assignment lhs' in those bbs were dependent on the conditions > > + guarding those basic blocks which now can change, the VRs might > > + be incorrect. As no_side_effect_bb should ensure those SSA_NAMEs > > + are only used within the same bb, it should be not a big deal if > > + we just reset all the VRs in those bbs. See PR68671. */ > > + for (bb = last_bb, idx = 0; idx < max_idx; bb = single_pred (bb), idx++) > > + { > > + gimple_stmt_iterator gsi; > > + for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi)) > > + { > > + gimple *g = gsi_stmt (gsi); > > + if (!is_gimple_assign (g)) > > + continue; > > + tree lhs = gimple_assign_lhs (g); > > + if (TREE_CODE (lhs) != SSA_NAME) > > + continue; > > + if (INTEGRAL_TYPE_P (TREE_TYPE (lhs))) > > + SSA_NAME_RANGE_INFO (lhs) = NULL; > > Please use > > reset_flow_sensitive_info (lhs); So maybe better then replace the whole inner loop with reset_flow_sensitive_info_in_bb (bb); ? Jakub
On Fri, 4 Dec 2015, Jakub Jelinek wrote: > On Fri, Dec 04, 2015 at 09:15:25AM +0100, Richard Biener wrote: > > > + modified one, up to and including last_bb, to be executed even if > > > + they would not be in the original program. If the value ranges of > > > + assignment lhs' in those bbs were dependent on the conditions > > > + guarding those basic blocks which now can change, the VRs might > > > + be incorrect. As no_side_effect_bb should ensure those SSA_NAMEs > > > + are only used within the same bb, it should be not a big deal if > > > + we just reset all the VRs in those bbs. See PR68671. */ > > > + for (bb = last_bb, idx = 0; idx < max_idx; bb = single_pred (bb), idx++) > > > + { > > > + gimple_stmt_iterator gsi; > > > + for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi)) > > > + { > > > + gimple *g = gsi_stmt (gsi); > > > + if (!is_gimple_assign (g)) > > > + continue; > > > + tree lhs = gimple_assign_lhs (g); > > > + if (TREE_CODE (lhs) != SSA_NAME) > > > + continue; > > > + if (INTEGRAL_TYPE_P (TREE_TYPE (lhs))) > > > + SSA_NAME_RANGE_INFO (lhs) = NULL; > > > > Please use > > > > reset_flow_sensitive_info (lhs); > > So maybe better then replace the whole inner loop with > reset_flow_sensitive_info_in_bb (bb); > ? Yeah, indeed. I was confused about the max_id stuff and read you were handling some blocks partially only. Richard.
--- gcc/tree-ssa-reassoc.c.jj 2015-11-18 11:22:51.000000000 +0100 +++ gcc/tree-ssa-reassoc.c 2015-12-03 18:12:08.915210122 +0100 @@ -3204,7 +3204,7 @@ maybe_optimize_range_tests (gimple *stmt any_changes = optimize_range_tests (ERROR_MARK, &ops); if (any_changes) { - unsigned int idx; + unsigned int idx, max_idx = 0; /* update_ops relies on has_single_use predicates returning the same values as it did during get_ops earlier. Additionally it never removes statements, only adds new ones and it should walk @@ -3220,6 +3220,7 @@ maybe_optimize_range_tests (gimple *stmt { tree new_op; + max_idx = idx; stmt = last_stmt (bb); new_op = update_ops (bbinfo[idx].op, (enum tree_code) @@ -3289,6 +3290,10 @@ maybe_optimize_range_tests (gimple *stmt && ops[bbinfo[idx].first_idx]->op != NULL_TREE) { gcond *cond_stmt = as_a <gcond *> (last_stmt (bb)); + + if (idx > max_idx) + max_idx = idx; + if (integer_zerop (ops[bbinfo[idx].first_idx]->op)) gimple_cond_make_false (cond_stmt); else if (integer_onep (ops[bbinfo[idx].first_idx]->op)) @@ -3305,6 +3310,30 @@ maybe_optimize_range_tests (gimple *stmt if (bb == first_bb) break; } + + /* The above changes could result in basic blocks after the first + modified one, up to and including last_bb, to be executed even if + they would not be in the original program. If the value ranges of + assignment lhs' in those bbs were dependent on the conditions + guarding those basic blocks which now can change, the VRs might + be incorrect. As no_side_effect_bb should ensure those SSA_NAMEs + are only used within the same bb, it should be not a big deal if + we just reset all the VRs in those bbs. See PR68671. */ + for (bb = last_bb, idx = 0; idx < max_idx; bb = single_pred (bb), idx++) + { + gimple_stmt_iterator gsi; + for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi)) + { + gimple *g = gsi_stmt (gsi); + if (!is_gimple_assign (g)) + continue; + tree lhs = gimple_assign_lhs (g); + if (TREE_CODE (lhs) != SSA_NAME) + continue; + if (INTEGRAL_TYPE_P (TREE_TYPE (lhs))) + SSA_NAME_RANGE_INFO (lhs) = NULL; + } + } } } --- gcc/testsuite/gcc.dg/pr68671.c.jj 2015-12-03 18:19:24.769104484 +0100 +++ gcc/testsuite/gcc.dg/pr68671.c 2015-12-03 18:19:07.000000000 +0100 @@ -0,0 +1,23 @@ +/* PR tree-optimization/68671 */ +/* { dg-do run } */ +/* { dg-options " -O2 -fno-tree-dce" } */ + +volatile int a = -1; +volatile int b; + +static inline int +fn1 (signed char p1, int p2) +{ + return (p1 < 0) || (p1 > (1 >> p2)) ? 0 : (p1 << 1); +} + +int +main () +{ + signed char c = a; + b = fn1 (c, 1); + c = ((128 | c) < 0 ? 1 : 0); + if (c != 1) + __builtin_abort (); + return 0; +}