| Message ID | 1374972662.3633.164.camel@gnopaine |
|---|---|
| State | New |
| Headers | show |
On 07/27/2013 06:51 PM, Bill Schmidt wrote: > PR57993 reports a scenario where a conditional candidate is incorrectly > replaced when its putative "hidden basis" (the basis hidden by one or > more PHI nodes) does not dominate all of the PHI nodes on which the > candidate depends. > > This indicates that the hidden basis was incorrectly identified as a > useful basis for the candidate. There are two ways we can fix this. > One would be to add more complexity to the code that determines the > hidden basis. Currently that code does not recursively follow the PHI > definitions backwards to ensure that the basis dominates all the PHIs. > Adding code to do this would ensure we didn't identify an incorrect > basis in the first place, but would duplicate quite a bit of existing > logic in the later analysis phase, and increase compile time. > > What I've done here is to instead delay detection of the problem until > the analysis phase. If we detect that we chose an invalid basis, we > assign an "infinite" cost to the replacement so that we won't pursue it > further. > > This requires that we know which basic block contains the basis > statement. The basis statement may have itself been replaced by another > statement earlier, so we need to keep the candidate table up to date > with respect to such replacements. > > Bootstrapped and tested on powerpc64-linux-gnu with no new regressions. > Ok for trunk? > > Thanks, > Bill > > > gcc: > > 2013-07-27 Bill Schmidt <wschmidt@vnet.linux.ibm.com> > > * gimple-ssa-strength-reduction.c (replace_mult_candidate): Record > replaced statement in the candidate table. > (phi_add_costs): Return infinite cost when the hidden basis does > not dominate all phis on which the candidate is dependent. > (replace_one_candidate): Record replaced statement in the > candidate table. > > gcc/testsuite: > > 2013-07-27 Bill Schmidt <wschmidt@vnet.linux.ibm.com> > > * gcc.dg/torture/pr57993.c: New test. Looks good. Please install! Thanks, Jeff
Index: gcc/testsuite/gcc.dg/torture/pr57993.c =================================================================== --- gcc/testsuite/gcc.dg/torture/pr57993.c (revision 0) +++ gcc/testsuite/gcc.dg/torture/pr57993.c (revision 0) @@ -0,0 +1,30 @@ +/* This ICEd prior to fixing PR57993. */ +/* { dg-do compile } */ + +int a, b, c, d; +char e; +unsigned g; + +void f(void) +{ + int h; + + for(; d; d++) + if(d) +lbl: + g + a || (d = 0); + + b && (a = e); + + for(h = 0; h < 1; ++h) + { + h = c ? : (d = 0); + g = a = (e | 0); + } + + if(a) + goto lbl; + + a = e = 0; + goto lbl; +} Index: gcc/gimple-ssa-strength-reduction.c =================================================================== --- gcc/gimple-ssa-strength-reduction.c (revision 201267) +++ gcc/gimple-ssa-strength-reduction.c (working copy) @@ -1882,6 +1882,7 @@ replace_mult_candidate (slsr_cand_t c, tree basis_ gimple_stmt_iterator gsi = gsi_for_stmt (c->cand_stmt); gimple_set_location (copy_stmt, gimple_location (c->cand_stmt)); gsi_replace (&gsi, copy_stmt, false); + c->cand_stmt = copy_stmt; if (dump_file && (dump_flags & TDF_DETAILS)) stmt_to_print = copy_stmt; } @@ -2179,6 +2180,18 @@ phi_add_costs (gimple phi, slsr_cand_t c, int one_ int cost = 0; slsr_cand_t phi_cand = base_cand_from_table (gimple_phi_result (phi)); + /* If we work our way back to a phi that isn't dominated by the hidden + basis, this isn't a candidate for replacement. Indicate this by + returning an unreasonably high cost. It's not easy to detect + these situations when determining the basis, so we defer the + decision until now. */ + basic_block phi_bb = gimple_bb (phi); + slsr_cand_t basis = lookup_cand (c->basis); + basic_block basis_bb = gimple_bb (basis->cand_stmt); + + if (phi_bb == basis_bb || !dominated_by_p (CDI_DOMINATORS, phi_bb, basis_bb)) + return COST_INFINITE; + for (i = 0; i < gimple_phi_num_args (phi); i++) { tree arg = gimple_phi_arg_def (phi, i); @@ -3226,6 +3239,7 @@ replace_one_candidate (slsr_cand_t c, unsigned i, gimple_stmt_iterator gsi = gsi_for_stmt (c->cand_stmt); gimple_set_location (copy_stmt, gimple_location (c->cand_stmt)); gsi_replace (&gsi, copy_stmt, false); + c->cand_stmt = copy_stmt; if (dump_file && (dump_flags & TDF_DETAILS)) stmt_to_print = copy_stmt; @@ -3238,6 +3252,7 @@ replace_one_candidate (slsr_cand_t c, unsigned i, NULL_TREE); gimple_set_location (cast_stmt, gimple_location (c->cand_stmt)); gsi_replace (&gsi, cast_stmt, false); + c->cand_stmt = cast_stmt; if (dump_file && (dump_flags & TDF_DETAILS)) stmt_to_print = cast_stmt;