Patchwork Fix PR54674

login
register
mail settings
Submitter William J. Schmidt
Date Sept. 24, 2012, 8:38 p.m.
Message ID <1348519113.27229.34.camel@gnopaine>
Download mbox | patch
Permalink /patch/186560/
State New
Headers show

Comments

William J. Schmidt - Sept. 24, 2012, 8:38 p.m.
In cases where pointers and ints are cast back and forth, SLSR can be
tricked into introducing a multiply where one of the operands is of
pointer type.  Don't do that!

Verified that the reduced test case in the PR is fixed with a
cross-compile to sh4-unknown-linux-gnu with -Os, which is the only known
situation where the replacement looks profitable.  (It appears multiply
costs are underestimated.)

Bootstrapped and tested on powerpc64-unknown-linux-gnu with no new
regressions.  Ok for trunk?

Thanks,
Bill


2012-09-24  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	* gimple-ssa-strength-reduction.c (analyze_increments): Don't
	introduce a multiplication with a pointer operand.
Richard Guenther - Sept. 25, 2012, 7:14 a.m.
On Mon, 24 Sep 2012, William J. Schmidt wrote:

> In cases where pointers and ints are cast back and forth, SLSR can be
> tricked into introducing a multiply where one of the operands is of
> pointer type.  Don't do that!
> 
> Verified that the reduced test case in the PR is fixed with a
> cross-compile to sh4-unknown-linux-gnu with -Os, which is the only known
> situation where the replacement looks profitable.  (It appears multiply
> costs are underestimated.)
> 
> Bootstrapped and tested on powerpc64-unknown-linux-gnu with no new
> regressions.  Ok for trunk?

Ok.  Btw, a multiply by/of a pointer in GIMPLE is done by casting
to an appropriate unsigned type, doing the multiply, and then
casting back to the pointer type.  Just in case it _is_ profitable
to do the transform (the patch seems to try to avoid the situation
only?)

Thanks,
Richard.

> Thanks,
> Bill
> 
> 
> 2012-09-24  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
> 
> 	* gimple-ssa-strength-reduction.c (analyze_increments): Don't
> 	introduce a multiplication with a pointer operand.
> 
> 
> Index: gcc/gimple-ssa-strength-reduction.c
> ===================================================================
> --- gcc/gimple-ssa-strength-reduction.c	(revision 191665)
> +++ gcc/gimple-ssa-strength-reduction.c	(working copy)
> @@ -2028,6 +2028,15 @@ analyze_increments (slsr_cand_t first_dep, enum ma
>  
>  	incr_vec[i].cost = COST_INFINITE;
>  
> +      /* If we need to add an initializer, make sure we don't introduce
> +	 a multiply by a pointer type, which can happen in certain cast
> +	 scenarios.  */
> +      else if (!incr_vec[i].initializer
> +	       && TREE_CODE (first_dep->stride) != INTEGER_CST
> +	       && POINTER_TYPE_P (TREE_TYPE (first_dep->stride)))
> +
> +	incr_vec[i].cost = COST_INFINITE;
> +
>        /* For any other increment, if this is a multiply candidate, we
>  	 must introduce a temporary T and initialize it with
>  	 T_0 = stride * increment.  When optimizing for speed, walk the
> 
> 
>
William J. Schmidt - Sept. 25, 2012, 12:35 p.m.
On Tue, 2012-09-25 at 09:14 +0200, Richard Guenther wrote:
> On Mon, 24 Sep 2012, William J. Schmidt wrote:
> 
> > In cases where pointers and ints are cast back and forth, SLSR can be
> > tricked into introducing a multiply where one of the operands is of
> > pointer type.  Don't do that!
> > 
> > Verified that the reduced test case in the PR is fixed with a
> > cross-compile to sh4-unknown-linux-gnu with -Os, which is the only known
> > situation where the replacement looks profitable.  (It appears multiply
> > costs are underestimated.)
> > 
> > Bootstrapped and tested on powerpc64-unknown-linux-gnu with no new
> > regressions.  Ok for trunk?
> 
> Ok.  Btw, a multiply by/of a pointer in GIMPLE is done by casting
> to an appropriate unsigned type, doing the multiply, and then
> casting back to the pointer type.  Just in case it _is_ profitable
> to do the transform (the patch seems to try to avoid the situation
> only?)

Ok, that's good to know, thanks.  There's a general to-do in that area
to make the whole casting part better than it is right now, and that
should be addressed when I can get back to GCC and work on some of these
things.  I'll add a comment to that effect.  Appreciate the information!

Thanks,
Bill

> 
> Thanks,
> Richard.
> 
> > Thanks,
> > Bill
> > 
> > 
> > 2012-09-24  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
> > 
> > 	* gimple-ssa-strength-reduction.c (analyze_increments): Don't
> > 	introduce a multiplication with a pointer operand.
> > 
> > 
> > Index: gcc/gimple-ssa-strength-reduction.c
> > ===================================================================
> > --- gcc/gimple-ssa-strength-reduction.c	(revision 191665)
> > +++ gcc/gimple-ssa-strength-reduction.c	(working copy)
> > @@ -2028,6 +2028,15 @@ analyze_increments (slsr_cand_t first_dep, enum ma
> >  
> >  	incr_vec[i].cost = COST_INFINITE;
> >  
> > +      /* If we need to add an initializer, make sure we don't introduce
> > +	 a multiply by a pointer type, which can happen in certain cast
> > +	 scenarios.  */
> > +      else if (!incr_vec[i].initializer
> > +	       && TREE_CODE (first_dep->stride) != INTEGER_CST
> > +	       && POINTER_TYPE_P (TREE_TYPE (first_dep->stride)))
> > +
> > +	incr_vec[i].cost = COST_INFINITE;
> > +
> >        /* For any other increment, if this is a multiply candidate, we
> >  	 must introduce a temporary T and initialize it with
> >  	 T_0 = stride * increment.  When optimizing for speed, walk the
> > 
> > 
> > 
>

Patch

Index: gcc/gimple-ssa-strength-reduction.c
===================================================================
--- gcc/gimple-ssa-strength-reduction.c	(revision 191665)
+++ gcc/gimple-ssa-strength-reduction.c	(working copy)
@@ -2028,6 +2028,15 @@  analyze_increments (slsr_cand_t first_dep, enum ma
 
 	incr_vec[i].cost = COST_INFINITE;
 
+      /* If we need to add an initializer, make sure we don't introduce
+	 a multiply by a pointer type, which can happen in certain cast
+	 scenarios.  */
+      else if (!incr_vec[i].initializer
+	       && TREE_CODE (first_dep->stride) != INTEGER_CST
+	       && POINTER_TYPE_P (TREE_TYPE (first_dep->stride)))
+
+	incr_vec[i].cost = COST_INFINITE;
+
       /* For any other increment, if this is a multiply candidate, we
 	 must introduce a temporary T and initialize it with
 	 T_0 = stride * increment.  When optimizing for speed, walk the