diff mbox series

[rs6000] Fix PR85755: PowerPC Gcc's -mupdate produces inefficient code

Message ID 262c83bc-5ced-37c5-3afe-b7a31cc210a3@vnet.ibm.com
State New
Headers show
Series [rs6000] Fix PR85755: PowerPC Gcc's -mupdate produces inefficient code | expand

Commit Message

Peter Bergner June 7, 2018, 10:12 p.m. UTC 
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.

Comments

Peter Bergner June 8, 2018, 1:16 a.m. UTC | #1 
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
Peter Bergner June 8, 2018, 3:35 p.m. UTC | #2 
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} } } */
Segher Boessenkool June 8, 2018, 4:21 p.m. UTC | #3 
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
Peter Bergner June 8, 2018, 5:07 p.m. UTC | #4 
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} } } */
Segher Boessenkool June 8, 2018, 5:12 p.m. UTC | #5 
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
Peter Bergner June 8, 2018, 5:19 p.m. UTC | #6 
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
diff mbox series

Patch

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} } } */