Combine simplify_set WORD_REGISTER_OPERATIONS

Combine simplify_set WORD_REGISTER_OPERATIONS

[Alan Modra]

The comment says this test is supposed to prevent "a narrower
operation than requested", but it actually only allows a larger
subreg, not one the same size.  Fix that.

Bootstrapped and regression tested powerpc64-linux.  OK for stage1?

Note that this bug was found when investigating why gcc-6 does not
suffer from pr69548, ie. this bug was masking a powerpc backend bug.

	* combine.c (simplify_set): Correct WORD_REGISTER_OPERATIONS test.

diff --git a/gcc/combine.c b/gcc/combine.c
index 858552d..9f284a7 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -6736,7 +6736,7 @@ simplify_set (rtx x)
 	       + (UNITS_PER_WORD - 1)) / UNITS_PER_WORD))
       && (WORD_REGISTER_OPERATIONS
 	  || (GET_MODE_SIZE (GET_MODE (src))
-	      < GET_MODE_SIZE (GET_MODE (SUBREG_REG (src)))))
+	      <= GET_MODE_SIZE (GET_MODE (SUBREG_REG (src)))))
 #ifdef CANNOT_CHANGE_MODE_CLASS
       && ! (REG_P (dest) && REGNO (dest) < FIRST_PSEUDO_REGISTER
 	    && REG_CANNOT_CHANGE_MODE_P (REGNO (dest),

-- 
Alan Modra
Australia Development Lab, IBM

Re: Combine simplify_set WORD_REGISTER_OPERATIONS

[Segher Boessenkool]

On Mon, Feb 01, 2016 at 08:46:42AM +1030, Alan Modra wrote:
> The comment says this test is supposed to prevent "a narrower
> operation than requested", but it actually only allows a larger
> subreg, not one the same size.  Fix that.
> 
> Bootstrapped and regression tested powerpc64-linux.  OK for stage1?
> 
> Note that this bug was found when investigating why gcc-6 does not
> suffer from pr69548, ie. this bug was masking a powerpc backend bug.

It sounds like you have a testcase, can we see it please?

And, just a missed optimisation, not a bug, right?


Segher

> 	* combine.c (simplify_set): Correct WORD_REGISTER_OPERATIONS test.
> 
> diff --git a/gcc/combine.c b/gcc/combine.c
> index 858552d..9f284a7 100644
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -6736,7 +6736,7 @@ simplify_set (rtx x)
>  	       + (UNITS_PER_WORD - 1)) / UNITS_PER_WORD))
>        && (WORD_REGISTER_OPERATIONS
>  	  || (GET_MODE_SIZE (GET_MODE (src))
> -	      < GET_MODE_SIZE (GET_MODE (SUBREG_REG (src)))))
> +	      <= GET_MODE_SIZE (GET_MODE (SUBREG_REG (src)))))
>  #ifdef CANNOT_CHANGE_MODE_CLASS
>        && ! (REG_P (dest) && REGNO (dest) < FIRST_PSEUDO_REGISTER
>  	    && REG_CANNOT_CHANGE_MODE_P (REGNO (dest),

Re: Combine simplify_set WORD_REGISTER_OPERATIONS

[Alan Modra]

On Sun, Jan 31, 2016 at 06:02:35PM -0600, Segher Boessenkool wrote:
> On Mon, Feb 01, 2016 at 08:46:42AM +1030, Alan Modra wrote:
> > The comment says this test is supposed to prevent "a narrower
> > operation than requested", but it actually only allows a larger
> > subreg, not one the same size.  Fix that.
> > 
> > Bootstrapped and regression tested powerpc64-linux.  OK for stage1?
> > 
> > Note that this bug was found when investigating why gcc-6 does not
> > suffer from pr69548, ie. this bug was masking a powerpc backend bug.
> 
> It sounds like you have a testcase, can we see it please?

The testcase in pr69548 will show changes in rtl..

> And, just a missed optimisation, not a bug, right?

Yes, not a bug, and only presumed a missed optimisation.  I don't
actually have a testcase that shows worse code.  All I have is a
comment that makes sense to me, that doesn't agree exactly with the
code, and some understanding how the code may have been accidentally
written the way it is.

-- 
Alan Modra
Australia Development Lab, IBM

Re: Combine simplify_set WORD_REGISTER_OPERATIONS

[Jeff Law]

On 01/31/2016 03:16 PM, Alan Modra wrote:
> The comment says this test is supposed to prevent "a narrower
> operation than requested", but it actually only allows a larger
> subreg, not one the same size.  Fix that.
>
> Bootstrapped and regression tested powerpc64-linux.  OK for stage1?
>
> Note that this bug was found when investigating why gcc-6 does not
> suffer from pr69548, ie. this bug was masking a powerpc backend bug.
>
> 	* combine.c (simplify_set): Correct WORD_REGISTER_OPERATIONS test.
Is there a strong need to apply this to gcc6?  Can we construct a 
testcase where this makes a difference in the code we generate?

My inclination would be to approve for gcc-7 as-is, but I'm more 
hesitant for gcc-6.

jeff

Re: Combine simplify_set WORD_REGISTER_OPERATIONS

[Alan Modra]

On Mon, Feb 08, 2016 at 09:27:36AM -0700, Jeff Law wrote:
> On 01/31/2016 03:16 PM, Alan Modra wrote:
> >The comment says this test is supposed to prevent "a narrower
> >operation than requested", but it actually only allows a larger
> >subreg, not one the same size.  Fix that.
> >
> >Bootstrapped and regression tested powerpc64-linux.  OK for stage1?
> >
> >Note that this bug was found when investigating why gcc-6 does not
> >suffer from pr69548, ie. this bug was masking a powerpc backend bug.
> >
> >	* combine.c (simplify_set): Correct WORD_REGISTER_OPERATIONS test.
> 
> Is there a strong need to apply this to gcc6?

No, better to wait for gcc-7, I think.

>  Can we construct a testcase
> where this makes a difference in the code we generate?

I instrumented the combine.c code in question with this

      if (!WORD_REGISTER_OPERATIONS
	  && (GET_MODE_SIZE (GET_MODE (src))
	      == GET_MODE_SIZE (GET_MODE (SUBREG_REG (src)))))
	{
	  FILE *f = fopen ("/tmp/alan", "a");
	  fprintf (f, "%s\n", main_input_filename);
	  print_inline_rtx (f, src, 0);
	  fprintf (f, "\n");
	  fclose (f);
	}

to see what popped out when bootstrapping gcc on x86_64.  There were
quite a lot of hits, DI -> DF, SI -> SF, V4SF -> TI etc, especially in
the testsuite (I should have dumped options too)..  

Here's the first one:
/src/gcc.git/libgcc/config/libbid/bid64_div.c
(subreg:DF (plus:DI (subreg:DI (reg:DF 841) 0)
        (const_int 1 [0x1])) 0)
This one resulted in using lea vs. add, so slightly better code.

One from the testsuite:
/src/gcc.git/gcc/testsuite/gcc.dg/sso/p4.c
(subreg:SF (bswap:SI (reg:SI 99 [ Local_R2.F ])) 0)
When compiling with -Og, this showed

	before				after
	.loc 3 49 0			.loc 3 49 0
	movl	-32(%ebp), %eax		movl	-32(%ebp), %eax
	bswap	%eax			bswap	%eax
	movl	%eax, -44(%ebp)		movl	%eax, -28(%ebp)
	flds	-44(%ebp)
	fstps	-28(%ebp)

Quite an improvement, if you care about -Og code.

I didn't see any worse code, except some cases that I think were
caused by register allocation differences.

> My inclination would be to approve for gcc-7 as-is, but I'm more hesitant
> for gcc-6.
> 
> jeff

-- 
Alan Modra
Australia Development Lab, IBM

Re: Combine simplify_set WORD_REGISTER_OPERATIONS

[Segher Boessenkool]

On Mon, Feb 01, 2016 at 08:46:42AM +1030, Alan Modra wrote:
> The comment says this test is supposed to prevent "a narrower
> operation than requested", but it actually only allows a larger
> subreg, not one the same size.  Fix that.
> 
> Bootstrapped and regression tested powerpc64-linux.  OK for stage1?

It looks good, but please post it again then.


Segher

Re: Combine simplify_set WORD_REGISTER_OPERATIONS

[Jeff Law]

On 01/31/2016 03:16 PM, Alan Modra wrote:
> The comment says this test is supposed to prevent "a narrower
> operation than requested", but it actually only allows a larger
> subreg, not one the same size.  Fix that.
>
> Bootstrapped and regression tested powerpc64-linux.  OK for stage1?
>
> Note that this bug was found when investigating why gcc-6 does not
> suffer from pr69548, ie. this bug was masking a powerpc backend bug.
>
> 	* combine.c (simplify_set): Correct WORD_REGISTER_OPERATIONS test.
This is fine.  Please install.

THanks,
Jeff