| Message ID | 20160131221641.GV17028@bubble.grove.modra.org |
|---|---|
| State | New |
| Headers | show |
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),
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.
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
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
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
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
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),