Patchwork Fix SLSR conditional candidate bug with commuted operands

login
register
mail settings
Submitter William J. Schmidt
Date May 6, 2013, 10:16 p.m.
Message ID <1367878586.11638.72.camel@gnopaine>
Download mbox | patch
Permalink /patch/241804/
State New
Headers show

Comments

William J. Schmidt - May 6, 2013, 10:16 p.m.
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.
Richard Guenther - May 7, 2013, 8:32 a.m.
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);
>
>

Patch

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);