Message ID | 1452895600.14295.13.camel@gnopaine |
---|---|
State | New |
Headers | show |
On January 15, 2016 11:06:40 PM GMT+01:00, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote: >Hi, > >In straight-line strength reduction, it can sometimes happen that more >than one conditional candidate can be predicated upon a PHI node P. >During processing of the first conditional candidate, a new PHI node >may >be introduced in the same block as P, with new computations introduced >in predecessors of that block. If this requires splitting one or more >edges, the PHI statement will change, and as a result it will now have >a >different address in storage than it did before. > >A problem arose because we cached the addresses of the PHI statements >in >a mapping from statements to strength-reduction candidates. If, after >a >PHI statement changes, we refer to it by its new address when >consulting >this mapping, we won't find the associated candidate. > >Now, this shouldn't happen, because after creating the candidate table, >we should always refer to PHIs by the original address at the time of >the table's creation. However, I had some sloppy code that caused us >to >look up the PHI statement by its result operand, even though we already >had a handle to the original cached PHI statement. When that code is >used, it can find the moved statement instead, and things go wrong. > >This patch solves the problem by simply replacing the sloppy code with >a >direct lookup based on the cached PHI statement address, so everything >remains consistent. > >I haven't added a test case because this is a pretty difficult scenario >to re-create reliably. The patch is pretty obvious, so I'm not too >concerned about that. > >The problem was found originally by Matthias Klose while doing a >PGO/LTO >build of python-2.7, and reported as PR68799. Matthias has tested the >patch for me in his environment, and indeed it fixes the problem. I've >bootstrapped and tested on powerpc64le-unknown-linux-gnu with no >regressions for GCC 5.3. Currently doing the same for trunk. > >Assuming no regressions is this ok for mainline, and thereafter for GCC >5 and GCC 4.9? OK. Thanks, Richard. >Thanks, >Bill > > >2016-01-15 Bill Schmidt <wschmidt@linux.vnet.ibm.com> > > PR tree-optimization/68799 > * gimple-ssa-strength-reduction.c (create_phi_basis): Directly > look up phi candidates in the statement-candidate map. > (phi_add_costs): Likewise. > (record_phi_increments): Likewise. > (phi_incr_cost): Likewise. > (ncd_with_phi): Likewise. > (all_phi_incrs_profitable): Likewise. > > >Index: gcc/gimple-ssa-strength-reduction.c >=================================================================== >--- gcc/gimple-ssa-strength-reduction.c (revision 232394) >+++ gcc/gimple-ssa-strength-reduction.c (working copy) >@@ -2267,7 +2267,7 @@ create_phi_basis (slsr_cand_t c, gimple from_phi, > slsr_cand_t basis = lookup_cand (c->basis); > int nargs = gimple_phi_num_args (from_phi); > basic_block phi_bb = gimple_bb (from_phi); >- slsr_cand_t phi_cand = base_cand_from_table (gimple_phi_result >(from_phi)); >+ slsr_cand_t phi_cand = *stmt_cand_map->get (from_phi); > phi_args.create (nargs); > > /* Process each argument of the existing phi that represents >@@ -2376,7 +2376,7 @@ phi_add_costs (gimple phi, slsr_cand_t c, int >one_ > { > unsigned i; > int cost = 0; >- slsr_cand_t phi_cand = base_cand_from_table (gimple_phi_result >(phi)); >+ slsr_cand_t phi_cand = *stmt_cand_map->get (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 >@@ -2587,7 +2587,7 @@ static void > record_phi_increments (slsr_cand_t basis, gimple phi) > { > unsigned i; >- slsr_cand_t phi_cand = base_cand_from_table (gimple_phi_result >(phi)); >+ slsr_cand_t phi_cand = *stmt_cand_map->get (phi); > > for (i = 0; i < gimple_phi_num_args (phi); i++) > { >@@ -2658,7 +2658,7 @@ phi_incr_cost (slsr_cand_t c, const widest_int >&in > unsigned i; > int cost = 0; > slsr_cand_t basis = lookup_cand (c->basis); >- slsr_cand_t phi_cand = base_cand_from_table (gimple_phi_result >(phi)); >+ slsr_cand_t phi_cand = *stmt_cand_map->get (phi); > > for (i = 0; i < gimple_phi_num_args (phi); i++) > { >@@ -3002,7 +3002,7 @@ ncd_with_phi (slsr_cand_t c, const widest_int >&inc > { > unsigned i; > slsr_cand_t basis = lookup_cand (c->basis); >- slsr_cand_t phi_cand = base_cand_from_table (gimple_phi_result >(phi)); >+ slsr_cand_t phi_cand = *stmt_cand_map->get (phi); > > for (i = 0; i < gimple_phi_num_args (phi); i++) > { >@@ -3212,7 +3212,7 @@ all_phi_incrs_profitable (slsr_cand_t c, gimple >ph > { > unsigned i; > slsr_cand_t basis = lookup_cand (c->basis); >- slsr_cand_t phi_cand = base_cand_from_table (gimple_phi_result >(phi)); >+ slsr_cand_t phi_cand = *stmt_cand_map->get (phi); > > for (i = 0; i < gimple_phi_num_args (phi); i++) > {
Index: gcc/gimple-ssa-strength-reduction.c =================================================================== --- gcc/gimple-ssa-strength-reduction.c (revision 232394) +++ gcc/gimple-ssa-strength-reduction.c (working copy) @@ -2267,7 +2267,7 @@ create_phi_basis (slsr_cand_t c, gimple from_phi, slsr_cand_t basis = lookup_cand (c->basis); int nargs = gimple_phi_num_args (from_phi); basic_block phi_bb = gimple_bb (from_phi); - slsr_cand_t phi_cand = base_cand_from_table (gimple_phi_result (from_phi)); + slsr_cand_t phi_cand = *stmt_cand_map->get (from_phi); phi_args.create (nargs); /* Process each argument of the existing phi that represents @@ -2376,7 +2376,7 @@ phi_add_costs (gimple phi, slsr_cand_t c, int one_ { unsigned i; int cost = 0; - slsr_cand_t phi_cand = base_cand_from_table (gimple_phi_result (phi)); + slsr_cand_t phi_cand = *stmt_cand_map->get (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 @@ -2587,7 +2587,7 @@ static void record_phi_increments (slsr_cand_t basis, gimple phi) { unsigned i; - slsr_cand_t phi_cand = base_cand_from_table (gimple_phi_result (phi)); + slsr_cand_t phi_cand = *stmt_cand_map->get (phi); for (i = 0; i < gimple_phi_num_args (phi); i++) { @@ -2658,7 +2658,7 @@ phi_incr_cost (slsr_cand_t c, const widest_int &in unsigned i; int cost = 0; slsr_cand_t basis = lookup_cand (c->basis); - slsr_cand_t phi_cand = base_cand_from_table (gimple_phi_result (phi)); + slsr_cand_t phi_cand = *stmt_cand_map->get (phi); for (i = 0; i < gimple_phi_num_args (phi); i++) { @@ -3002,7 +3002,7 @@ ncd_with_phi (slsr_cand_t c, const widest_int &inc { unsigned i; slsr_cand_t basis = lookup_cand (c->basis); - slsr_cand_t phi_cand = base_cand_from_table (gimple_phi_result (phi)); + slsr_cand_t phi_cand = *stmt_cand_map->get (phi); for (i = 0; i < gimple_phi_num_args (phi); i++) { @@ -3212,7 +3212,7 @@ all_phi_incrs_profitable (slsr_cand_t c, gimple ph { unsigned i; slsr_cand_t basis = lookup_cand (c->basis); - slsr_cand_t phi_cand = base_cand_from_table (gimple_phi_result (phi)); + slsr_cand_t phi_cand = *stmt_cand_map->get (phi); for (i = 0; i < gimple_phi_num_args (phi); i++) {