| Message ID | 1367878586.11638.72.camel@gnopaine |
|---|---|
| State | New |
| Headers | show |
On Tue, May 7, 2013 at 12:16 AM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote: > This fixes the following bug: > > On Mon, 2013-05-06 at 21:25 +0200, Jakub Jelinek wrote: >> On Sun, May 05, 2013 at 03:45:17PM -0500, Bill Schmidt wrote: >> > 2013-05-05 Bill Schmidt <wschmidt@linux.vnet.ibm.com> >> > >> > * gimple-ssa-strength-reduction.c (slsr_process_phi): Re-enable. >> > (find_candidates_in_block): Re-enable slsr_process_phi. >> > (create_phi_basis): Fix double counting of candidate adjustment. >> >> This broke gcc.dg/pr33017.c testcase on i?86/x86_64 -m32. >> ./cc1 -O2 -ftree-vectorize -m32 -mno-sse pr33017.c >> difference between r19862{6,7} is: >> --- pr33017.s1 2013-05-06 21:22:03.786745422 +0200 >> +++ pr33017.s2 2013-05-06 21:22:16.844673015 +0200 >> @@ -32,9 +32,9 @@ foo: >> movb $87, var.1373+2(%eax) >> je .L10 >> cmpl $3, %edx >> - movb $87, var.1373+3(%eax) >> + movb $87, var.1373+2(%eax,%eax) >> jne .L11 >> - movb $87, var.1373+4(%eax) >> + movb $87, var.1373+2(%eax,%eax,2) >> movl $3, %ebp >> movl $61, 28(%esp) >> .L3: >> >> Jakub >> > > The pattern we seek for replacing a conditional candidate can be > confused when a CAND_ADD with an index of 1 has its operands commuted in > the analysis. (For x = y + z, we may record two CAND_ADDs, one for > either ordering of the addends.) For a candidate with reversed addends > in the analysis, don't attempt to find a hidden basis. Otherwise we > make an invalid replacement later on. > > Verified this fixes the reported bug. Bootstrapped and tested on > powerpc64-unknown-linux-gnu with no new regressions. Ok for trunk? > > Thanks, > Bill > > > 2013-05-06 Bill Schmidt <wschmidt@linux.vnet.ibm.com> > > * gimple-ssa-strength-reduction.c (find_phi_def): Don't record a > phi def as possibly hiding a basis for a CAND_ADD whose operands > have been commuted in the analysis. > (alloc_cand_and_find_basis): Add parms to call to find_phi_def. > > > Index: gcc/gimple-ssa-strength-reduction.c > =================================================================== > --- gcc/gimple-ssa-strength-reduction.c (revision 198627) > +++ gcc/gimple-ssa-strength-reduction.c (working copy) > @@ -413,15 +413,29 @@ cand_chain_hasher::equal (const value_type *chain1 > static hash_table <cand_chain_hasher> base_cand_map; > > /* Look in the candidate table for a CAND_PHI that defines BASE and > - return it if found; otherwise return NULL. */ > + return it if found; otherwise return NULL. GS is the candidate > + statement with BASE, INDEX, and STRIDE. If GS is a CAND_ADD with > + an index of 1 and an SSA name for STRIDE, we must be careful that > + we haven't commuted the operands for this candidate. STRIDE must > + correspond to the second addend of GS for the eventual transformation > + to be legal. If not, return NULL. */ > > static cand_idx > -find_phi_def (tree base) > +find_phi_def (gimple gs, enum cand_kind kind, tree base, > + double_int index, tree stride) > { > slsr_cand_t c; > > if (TREE_CODE (base) != SSA_NAME) > return 0; > + > + /* Check for commutativity condition. */ Please adjust the comment - the test checks more and bails out. So the comment should say why. Ok with that change. Thanks, Richard. > + if (kind == CAND_ADD > + && index.is_one () > + && TREE_CODE (stride) == SSA_NAME > + && gimple_assign_rhs_code (gs) == PLUS_EXPR > + && stride != gimple_assign_rhs2 (gs)) > + return 0; > > c = base_cand_from_table (base); > > @@ -565,7 +579,7 @@ alloc_cand_and_find_basis (enum cand_kind kind, gi > c->next_interp = 0; > c->dependent = 0; > c->sibling = 0; > - c->def_phi = find_phi_def (base); > + c->def_phi = find_phi_def (gs, kind, base, index, stride); > c->dead_savings = savings; > > cand_vec.safe_push (c); > >
Index: gcc/gimple-ssa-strength-reduction.c =================================================================== --- gcc/gimple-ssa-strength-reduction.c (revision 198627) +++ gcc/gimple-ssa-strength-reduction.c (working copy) @@ -413,15 +413,29 @@ cand_chain_hasher::equal (const value_type *chain1 static hash_table <cand_chain_hasher> base_cand_map; /* Look in the candidate table for a CAND_PHI that defines BASE and - return it if found; otherwise return NULL. */ + return it if found; otherwise return NULL. GS is the candidate + statement with BASE, INDEX, and STRIDE. If GS is a CAND_ADD with + an index of 1 and an SSA name for STRIDE, we must be careful that + we haven't commuted the operands for this candidate. STRIDE must + correspond to the second addend of GS for the eventual transformation + to be legal. If not, return NULL. */ static cand_idx -find_phi_def (tree base) +find_phi_def (gimple gs, enum cand_kind kind, tree base, + double_int index, tree stride) { slsr_cand_t c; if (TREE_CODE (base) != SSA_NAME) return 0; + + /* Check for commutativity condition. */ + if (kind == CAND_ADD + && index.is_one () + && TREE_CODE (stride) == SSA_NAME + && gimple_assign_rhs_code (gs) == PLUS_EXPR + && stride != gimple_assign_rhs2 (gs)) + return 0; c = base_cand_from_table (base); @@ -565,7 +579,7 @@ alloc_cand_and_find_basis (enum cand_kind kind, gi c->next_interp = 0; c->dependent = 0; c->sibling = 0; - c->def_phi = find_phi_def (base); + c->def_phi = find_phi_def (gs, kind, base, index, stride); c->dead_savings = savings; cand_vec.safe_push (c);