Re: [PATCH, rs6000] Fix PR85755: PowerPC Gcc's -mupdate produces inefficient code
On 6/8/18 12:12 PM, Segher Boessenkool wrote: > On Fri, Jun 08, 2018 at 12:07:34PM -0500, Peter Bergner wrote: >> On 6/8/18 11:21 AM, Segher Boessenkool wrote: >>> On Fri, Jun 08, 2018 at 10:35:22AM -0500, Peter Bergner wrote: +/* { dg-final { scan-assembler-times {\mstdu\M} 2 } } */ +/* { dg-final { scan-assembler-not {\mstfdu\M} } } */ >>> >>> Does this need p8 at all? Would it be better to just test without -mcpu=, >>> on just whatever default cpu is thrown at it? p8 is default for powerpc64le >>> so it will get plenty coverage. >>> >>> You do need an lp64 target btw. >> >> I guess I was just following what was reported in the bugzilla, but you >> are correct, we don't need -mcpu=power8. How about the following? > > Looks perfect, thanks! Ok, I committed the patch with the updated test case to trunk. I'll let that bake over the weekend before committing the backports to the GCC 7 and 8 release branches. Thanks! Peter
Re: [PATCH, rs6000] Fix PR85755: PowerPC Gcc's -mupdate produces inefficient code
On Fri, Jun 08, 2018 at 12:07:34PM -0500, Peter Bergner wrote: > On 6/8/18 11:21 AM, Segher Boessenkool wrote: > > On Fri, Jun 08, 2018 at 10:35:22AM -0500, Peter Bergner wrote: > >> +/* { dg-final { scan-assembler-times {\mstdu\M} 2 } } */ > >> +/* { dg-final { scan-assembler-not {\mstfdu\M} } } */ > > > > Does this need p8 at all? Would it be better to just test without -mcpu=, > > on just whatever default cpu is thrown at it? p8 is default for powerpc64le > > so it will get plenty coverage. > > > > You do need an lp64 target btw. > > I guess I was just following what was reported in the bugzilla, but you > are correct, we don't need -mcpu=power8. How about the following? Looks perfect, thanks! Segher
Re: [PATCH, rs6000] Fix PR85755: PowerPC Gcc's -mupdate produces inefficient code
On 6/8/18 11:21 AM, Segher Boessenkool wrote: > On Fri, Jun 08, 2018 at 10:35:22AM -0500, Peter Bergner wrote: >> +/* { dg-final { scan-assembler-times {\mstdu\M} 2 } } */ >> +/* { dg-final { scan-assembler-not {\mstfdu\M} } } */ > > Does this need p8 at all? Would it be better to just test without -mcpu=, > on just whatever default cpu is thrown at it? p8 is default for powerpc64le > so it will get plenty coverage. > > You do need an lp64 target btw. I guess I was just following what was reported in the bugzilla, but you are correct, we don't need -mcpu=power8. How about the following? Peter Index: pr85755.c === --- pr85755.c (nonexistent) +++ pr85755.c (working copy) @@ -0,0 +1,22 @@ +/* { dg-do compile { target { powerpc*-*-* } } } */ +/* { dg-options "-O1" } */ + +void +preinc (long *q, long n) +{ + long i; + for (i = 0; i < n; i++) +q[i] = i; +} + +void +predec (long *q, long n) +{ + long i; + for (i = n; i >= 0; i--) +q[i] = i; +} + +/* { dg-final { scan-assembler-times {\mstwu\M} 2 { target ilp32 } } } */ +/* { dg-final { scan-assembler-times {\mstdu\M} 2 { target lp64 } } } */ +/* { dg-final { scan-assembler-not {\mstfdu\M} } } */
Re: [PATCH, rs6000] Fix PR85755: PowerPC Gcc's -mupdate produces inefficient code
On Fri, Jun 08, 2018 at 10:35:22AM -0500, Peter Bergner wrote: > The fix for PR83969 accidentally disallowed PRE_INC and PRE_DEC addresses > from being matched for the Y constraint leading to poor code generation. > The old PRE_INC and PRE_DEC addresses were originally accepted via the > address_offset() call and test, but the fix for PR83969 added the test > for rs6000_offsettable_memref_p() and that doesn't accept PRE_INC/PRE_DEC. > > My earlier patch just tried moving the rs6000_offsettable_memref_p() call > to after the address_offset() call, but I now remember why I had it placed > before it. The problem was that the address_offset() call and test was > incorrectly accepting some non-offsetable addresses, so we need to test for > rs6000_offsettable_memref_p() first. However, rs6000_offsettable_memref_p() > doesn't accept PRE_INC/PRE_DEC addresses, so the fix used here is to just > test for them explicitly before the other tests, which fixes the reported > bug and doesn't regress the older bugs. > > Is this ok for trunk and after some trunk burn in, the GCC 8 and 7 release > branches where the earlier fixes were backported to? All bootstrap builds > completed and the testsuite runs all showed no regressions. Looks good, please commit. Modulo some testcase things: > --- gcc/testsuite/gcc.target/powerpc/pr85755.c(nonexistent) > +++ gcc/testsuite/gcc.target/powerpc/pr85755.c(working copy) > @@ -0,0 +1,24 @@ > +/* { dg-do compile { target { powerpc*-*-* } } } */ > +/* { dg-skip-if "" { powerpc*-*-darwin* } } */ > +/* { dg-require-effective-target powerpc_p8vector_ok } */ > +/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { > "-mcpu=power8" } } */ > +/* { dg-options "-O1 -mcpu=power8" } */ > + > +void > +preinc (long *q, long n) > +{ > + long i; > + for (i = 0; i < n; i++) > +q[i] = i; > +} > + > +void > +predec (long *q, long n) > +{ > + long i; > + for (i = n; i >= 0; i--) > +q[i] = i; > +} > + > +/* { dg-final { scan-assembler-times {\mstdu\M} 2 } } */ > +/* { dg-final { scan-assembler-not {\mstfdu\M} } } */ Does this need p8 at all? Would it be better to just test without -mcpu=, on just whatever default cpu is thrown at it? p8 is default for powerpc64le so it will get plenty coverage. You do need an lp64 target btw. Thanks! Segher
Re: [PATCH, rs6000] Fix PR85755: PowerPC Gcc's -mupdate produces inefficient code
On 6/7/18 8:16 PM, Peter Bergner wrote: > On 6/7/18 5:12 PM, Peter Bergner wrote: >> Is this ok for trunk and the release branches where the earlier fixes >> were backported to, assuming no bootstrap errors and the testsuite runs >> do not show any regressions? > > Hold off for now. I'm seeing a TImode issue I need to debug first. The fix for PR83969 accidentally disallowed PRE_INC and PRE_DEC addresses from being matched for the Y constraint leading to poor code generation. The old PRE_INC and PRE_DEC addresses were originally accepted via the address_offset() call and test, but the fix for PR83969 added the test for rs6000_offsettable_memref_p() and that doesn't accept PRE_INC/PRE_DEC. My earlier patch just tried moving the rs6000_offsettable_memref_p() call to after the address_offset() call, but I now remember why I had it placed before it. The problem was that the address_offset() call and test was incorrectly accepting some non-offsetable addresses, so we need to test for rs6000_offsettable_memref_p() first. However, rs6000_offsettable_memref_p() doesn't accept PRE_INC/PRE_DEC addresses, so the fix used here is to just test for them explicitly before the other tests, which fixes the reported bug and doesn't regress the older bugs. Is this ok for trunk and after some trunk burn in, the GCC 8 and 7 release branches where the earlier fixes were backported to? All bootstrap builds completed and the testsuite runs all showed no regressions. gcc/ PR target/85755 * config/rs6000/rs6000.c (mem_operand_gpr): Enable PRE_INC and PRE_DEC addresses. gcc/testsuite/ PR target/85755 * gcc.target/powerpc/pr85755.c: New test. Index: gcc/config/rs6000/rs6000.c === --- gcc/config/rs6000/rs6000.c (revision 261279) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -7997,6 +7997,13 @@ mem_operand_gpr (rtx op, machine_mode mo int extra; rtx addr = XEXP (op, 0); + /* PR85755: Allow PRE_INC and PRE_DEC addresses. */ + if (TARGET_UPDATE + && (GET_CODE (addr) == PRE_INC || GET_CODE (addr) == PRE_DEC) + && mode_supports_pre_incdec_p (mode) + && legitimate_indirect_address_p (XEXP (addr, 0), false)) +return true; + /* Don't allow non-offsettable addresses. See PRs 83969 and 84279. */ if (!rs6000_offsettable_memref_p (op, mode, false)) return false; Index: gcc/testsuite/gcc.target/powerpc/pr85755.c === --- gcc/testsuite/gcc.target/powerpc/pr85755.c (nonexistent) +++ gcc/testsuite/gcc.target/powerpc/pr85755.c (working copy) @@ -0,0 +1,24 @@ +/* { dg-do compile { target { powerpc*-*-* } } } */ +/* { dg-skip-if "" { powerpc*-*-darwin* } } */ +/* { dg-require-effective-target powerpc_p8vector_ok } */ +/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power8" } } */ +/* { dg-options "-O1 -mcpu=power8" } */ + +void +preinc (long *q, long n) +{ + long i; + for (i = 0; i < n; i++) +q[i] = i; +} + +void +predec (long *q, long n) +{ + long i; + for (i = n; i >= 0; i--) +q[i] = i; +} + +/* { dg-final { scan-assembler-times {\mstdu\M} 2 } } */ +/* { dg-final { scan-assembler-not {\mstfdu\M} } } */
Re: [PATCH, rs6000] Fix PR85755: PowerPC Gcc's -mupdate produces inefficient code
On 6/7/18 5:12 PM, Peter Bergner wrote: > Is this ok for trunk and the release branches where the earlier fixes > were backported to, assuming no bootstrap errors and the testsuite runs > do not show any regressions? Hold off for now. I'm seeing a TImode issue I need to debug first. Peter
[PATCH, rs6000] Fix PR85755: PowerPC Gcc's -mupdate produces inefficient code
The fix for PR83969 accidentally disallowed PRE_INC and PRE_DEC addresses from being matched for the Y constraint leading to poor code generation. The following patch resurrects the fix for PR84279 (early test for altivec addresses) and then moves the call to rs6000_offsettable_memref_p below the address_offset test, which is what allows PRE_INC/PRE_DEC addresses to be matched. Is this ok for trunk and the release branches where the earlier fixes were backported to, assuming no bootstrap errors and the testsuite runs do not show any regressions? Peter gcc/ PR target/85755 * config/rs6000/rs6000.c (mem_operand_gpr): Disable Altivec addresses. Move call to rs6000_offsettable_memref_p below call to address_offset to allow PRE_INC and PRE_DEC addresses. gcc/testsuite/ PR target/85755 * gcc.target/powerpc/pr85755.c: New test. Index: gcc/config/rs6000/rs6000.c === --- gcc/config/rs6000/rs6000.c (revision 261279) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -7997,15 +7997,19 @@ mem_operand_gpr (rtx op, machine_mode mo int extra; rtx addr = XEXP (op, 0); - /* Don't allow non-offsettable addresses. See PRs 83969 and 84279. */ - if (!rs6000_offsettable_memref_p (op, mode, false)) + /* PR84279: Don't allow altivec addresses like (mem (and (plus ...))). */ + if (GET_CODE (addr) == AND) return false; - op = address_offset (addr); - if (op == NULL_RTX) + rtx offset_op = address_offset (addr); + if (offset_op == NULL_RTX) return true; - offset = INTVAL (op); + /* PR83969: Don't allow non-offsettable addresses. */ + if (!rs6000_offsettable_memref_p (op, mode, false)) +return false; + + offset = INTVAL (offset_op); if (TARGET_POWERPC64 && (offset & 3) != 0) return false; Index: gcc/testsuite/gcc.target/powerpc/pr85755.c === --- gcc/testsuite/gcc.target/powerpc/pr85755.c (nonexistent) +++ gcc/testsuite/gcc.target/powerpc/pr85755.c (working copy) @@ -0,0 +1,24 @@ +/* { dg-do compile { target { powerpc*-*-* } } } */ +/* { dg-skip-if "" { powerpc*-*-darwin* } } */ +/* { dg-require-effective-target powerpc_p8vector_ok } */ +/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power8" } } */ +/* { dg-options "-O1 -mcpu=power8" } */ + +void +preinc (long *q, long n) +{ + long i; + for (i = 0; i < n; i++) +q[i] = i; +} + +void +predec (long *q, long n) +{ + long i; + for (i = n; i >= 0; i--) +q[i] = i; +} + +/* { dg-final { scan-assembler-times {\mstdu\M} 2 } } */ +/* { dg-final { scan-assembler-not {\mstfdu\M} } } */