Bug 85755 - PowerPC Gcc's -mupdate produces inefficient code on power8/power9 machines
Summary: PowerPC Gcc's -mupdate produces inefficient code on power8/power9 machines
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 9.0
: P3 normal
Target Milestone: ---
Assignee: Peter Bergner
URL: https://gcc.gnu.org/ml/gcc-patches/20...
Keywords:
Depends on:
Blocks:
 
Reported: 2018-05-11 23:19 UTC by Michael Meissner
Modified: 2019-02-08 10:37 UTC (History)
5 users (show)

See Also:
Host:
Target: powerpc
Build:
Known to work:
Known to fail:
Last reconfirmed: 2018-05-12 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Meissner 2018-05-11 23:19:23 UTC
If you compile the following code for -mcpu=power8 or -mcpu=power9, it wants to generate a PRE_INC loop.  However, the GPR movdi insn constraint uses the 'Y' constraint for store, due to the LD/STD instructions being ds-form instructions.  Ds-form instructions cannot have the bottom 2 bits set in the offset field.  Because the register allocator does not have a GPR store that it thinks can do a store with update, it does a direct move to the floating point registers, and does a floating point store with update there.

We should probably allow store with update and load with update forms (or disable DImode from generating the PRE_INC/PRE_DEC/PRE_MODIFY instructions):

Code:

void loop (long *q, long n)
{
  long i;

  for (i = 0; i < n; i++)
    q[i] = i;
}


The code generated is:

loop:
.LFB0:
        cmpdi 0,4,0
        blelr 0
        mtctr 4
        addi 3,3,-8
        li 9,0
        .p2align 4,,15
.L3:
        mtvsrd 0,9
        addi 9,9,1
        stfdu 0,8(3)
        bdnz .L3
        blr
Comment 1 Segher Boessenkool 2018-05-12 00:20:56 UTC
Before RA it is all just fine:

insn_cost 4 for    14: [++r121:DI]=r122:DI
      REG_INC r121:DI
insn_cost 4 for    15: r122:DI=r122:DI+0x1
insn_cost 0 for    30: {pc={(r129:DI!=0x1)?L16:pc};r129:DI=r129:DI-0x1;clobber scratch;clobber scratch;}
      REG_BR_PROB 955630228

IRA make a good choice:

Disposition:
    3:r121 l0     3    0:r122 l0     9    4:r125 l0     3    2:r126 l0     4
    5:r127 l0    68    1:r129 l0    66

and then LRA comes along:

         Choosing alt 6 in insn 14:  (0) ^m  (1) d {*movdi_internal64}
      Creating newreg=132 from oldreg=122, assigning class FLOAT_REGS to r132
   14: [++r121:DI]=r132:DI
      REG_INC r121:DI
    Inserting insn reload before:
   35: r132:DI=r122:DI

We want to have alt 0 instead (r -> YZ), which gives

            0 Non input pseudo reload: reject++
          alt=0,overall=7,losers=1,rld_nregs=0

but we get alt 6 (d -> ^m), which gives

          alt=6,overall=6,losers=1,rld_nregs=1

Putting the disparagement on the correct operand, i.e. as  ^d -> m  gives

            alt=6,overall=12,losers=1 -- refuse

and we should make that change; but yes we want an stdu, too.  Should "Y"
allow that?

GCC 7 did this fine; GCC 8 of a month ago, too.  What changed?
Comment 2 Segher Boessenkool 2018-05-12 00:21:53 UTC
That is, that GCC 8 did not do pre-increment, but it did no silliness
with float registers.
Comment 3 Segher Boessenkool 2018-05-12 00:35:35 UTC
Sigh, i forgot -mcpu=power8 on that last test.

GCC 7 was just fine, stdu, everything.

GCC 8 was bad already.
Comment 4 Segher Boessenkool 2018-05-12 10:38:14 UTC
This is caused by r258400, the fix for PR83969.
Comment 5 Peter Bergner 2018-06-07 14:19:29 UTC
Hmmm, it must be the extra use of rs6000_offsettable_memref_p() that disables the update forms.  I'll have a look.
Comment 6 Peter Bergner 2018-06-08 15:37:20 UTC
Updated patch submitted.
Comment 7 Peter Bergner 2018-06-08 17:18:17 UTC
Author: bergner
Date: Fri Jun  8 17:17:45 2018
New Revision: 261340

URL: https://gcc.gnu.org/viewcvs?rev=261340&root=gcc&view=rev
Log:
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.

Added:
    trunk/gcc/testsuite/gcc.target/powerpc/pr85755.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/rs6000/rs6000.c
    trunk/gcc/testsuite/ChangeLog
Comment 8 Segher Boessenkool 2018-06-11 15:49:46 UTC
Author: segher
Date: Mon Jun 11 15:48:48 2018
New Revision: 261435

URL: https://gcc.gnu.org/viewcvs?rev=261435&root=gcc&view=rev
Log:
rs6000: Put constraints on the correct operand in movdi (PR85755)

Some of the mov* patterns use ^ and $ constraint modifiers, which mean
give a penalty to this alternative if this operand needs a reload.  They
are meant here to give a penalty if a register operand needs reloading
(because it needs to be in a different kind of register), not when a
memory operand needs reloading (which is easy and cheap to do).

This patch fixes the movdi patterns.  This fixes PR85755.

The following are changed (name, old constraints, new constraints):
FPR store   ^m := d     m := ^d
FPR move    ^d := d     ^d := ^d
AVX store   ^wY := wb   wY := ^wb
AVX store   $Z := wv    Z := $wv
VSX move    ^wi := wi   ^wi := ^wi


	PR target/85755
	* config/rs6000/rs6000.md (*movdi_internal32): Put constraint modifiers
	on the correct operand.
	(*movdi_internal64): Ditto.

---
 gcc/config/rs6000/rs6000.md | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index a2605a0..f06591f 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -8567,16 +8567,16 @@ (define_insn_and_split "reload_gpr_from_vsxsf"
 
 (define_insn "*movdi_internal32"
   [(set (match_operand:DI 0 "nonimmediate_operand"
-         "=Y,        r,         r,         ^m,        ^d,         ^d,
-          r,         ^wY,       $Z,        ^wb,       $wv,        ^wi,
+         "=Y,        r,         r,         m,         ^d,         ^d,
+          r,         wY,        Z,         ^wb,       $wv,        ^wi,
           *wo,       *wo,       *wv,       *wi,       *wi,        *wv,
           *wv")
 
 	(match_operand:DI 1 "input_operand"
-          "r,        Y,         r,         d,         m,          d,
-           IJKnGHF,  wb,        wv,        wY,        Z,          wi,
-           Oj,       wM,        OjwM,      Oj,        wM,         wS,
-           wB"))]
+         "r,         Y,         r,         ^d,        m,          ^d,
+          IJKnGHF,   ^wb,       $wv,       wY,        Z,          ^wi,
+          Oj,        wM,        OjwM,      Oj,        wM,         wS,
+          wB"))]
 
   "! TARGET_POWERPC64
    && (gpc_reg_operand (operands[0], DImode)
@@ -8643,17 +8643,17 @@ (define_split
 (define_insn "*movdi_internal64"
   [(set (match_operand:DI 0 "nonimmediate_operand"
                "=YZ,       r,         r,         r,         r,          r,
-                ^m,        ^d,        ^d,        ^wY,       $Z,         $wb,
+                m,         ^d,        ^d,        wY,        Z,          $wb,
                 $wv,       ^wi,       *wo,       *wo,       *wv,        *wi,
                 *wi,       *wv,       *wv,       r,         *h,         *h,
                 ?*r,       ?*wg,      ?*r,       ?*wj")
 
 	(match_operand:DI 1 "input_operand"
-                "r,        YZ,        r,         I,         L,          nF,
-                 d,        m,         d,         wb,        wv,         wY,
-                 Z,        wi,        Oj,        wM,        OjwM,       Oj,
-                 wM,       wS,        wB,        *h,        r,          0,
-                 wg,       r,         wj,        r"))]
+               "r,         YZ,        r,         I,         L,          nF,
+                ^d,        m,         ^d,        ^wb,       $wv,        wY,
+                Z,         ^wi,       Oj,        wM,        OjwM,       Oj,
+                wM,        wS,        wB,        *h,        r,          0,
+                wg,        r,         wj,        r"))]
 
   "TARGET_POWERPC64
    && (gpc_reg_operand (operands[0], DImode)
Comment 9 Segher Boessenkool 2018-06-11 16:07:21 UTC
Author: segher
Date: Mon Jun 11 16:06:49 2018
New Revision: 261436

URL: https://gcc.gnu.org/viewcvs?rev=261436&root=gcc&view=rev
Log:
	Backport from trunk
	2018-06-11  Segher Boessenkool  <segher@kernel.crashing.org>

	PR target/85755
	* config/rs6000/rs6000.md (*movdi_internal32): Put constraint modifiers
	on the correct operand.
	(*movdi_internal64): Ditto.

Modified:
    branches/gcc-8-branch/gcc/ChangeLog
    branches/gcc-8-branch/gcc/config/rs6000/rs6000.md
Comment 10 Peter Bergner 2018-06-11 18:24:00 UTC
Author: bergner
Date: Mon Jun 11 18:23:28 2018
New Revision: 261441

URL: https://gcc.gnu.org/viewcvs?rev=261441&root=gcc&view=rev
Log:
gcc/
	Backport from mainline
	2018-06-08  Peter Bergner  <bergner@vnet.ibm.com>

	PR target/85755
	* config/rs6000/rs6000.c (mem_operand_gpr): Enable PRE_INC and PRE_DEC
	addresses.

gcc/testsuite/
	Backport from mainline
	2018-06-08  Peter Bergner  <bergner@vnet.ibm.com>

	PR target/85755
	* gcc.target/powerpc/pr85755.c: New test.

Added:
    branches/gcc-8-branch/gcc/testsuite/gcc.target/powerpc/pr85755.c
Modified:
    branches/gcc-8-branch/gcc/ChangeLog
    branches/gcc-8-branch/gcc/config/rs6000/rs6000.c
    branches/gcc-8-branch/gcc/testsuite/ChangeLog
Comment 11 Peter Bergner 2018-06-11 18:26:09 UTC
Author: bergner
Date: Mon Jun 11 18:25:37 2018
New Revision: 261442

URL: https://gcc.gnu.org/viewcvs?rev=261442&root=gcc&view=rev
Log:
gcc/
	Backport from mainline
	2018-06-08  Peter Bergner  <bergner@vnet.ibm.com>

	PR target/85755
	* config/rs6000/rs6000.c (mem_operand_gpr): Enable PRE_INC and PRE_DEC
	addresses.

gcc/testsuite/
	Backport from mainline
	2018-06-08  Peter Bergner  <bergner@vnet.ibm.com>

	PR target/85755
	* gcc.target/powerpc/pr85755.c: New test.

Added:
    branches/gcc-7-branch/gcc/testsuite/gcc.target/powerpc/pr85755.c
Modified:
    branches/gcc-7-branch/gcc/ChangeLog
    branches/gcc-7-branch/gcc/config/rs6000/rs6000.c
    branches/gcc-7-branch/gcc/testsuite/ChangeLog
Comment 12 Peter Bergner 2018-06-11 18:57:32 UTC
The PRE_INC and PRE_DEC changes have been backported to GCC 8 and GCC 7, so this is fixed everywhere.
Comment 13 Eric Botcazou 2019-02-08 10:37:47 UTC
Author: ebotcazou
Date: Fri Feb  8 10:37:15 2019
New Revision: 268670

URL: https://gcc.gnu.org/viewcvs?rev=268670&root=gcc&view=rev
Log:
	Backport from mainline
	2018-06-11  Segher Boessenkool  <segher@kernel.crashing.org>

	PR target/85755
	* config/rs6000/rs6000.md (*movdi_internal32): Put constraint modifiers
	on the correct operand.
	(*movdi_internal64): Ditto.

Modified:
    branches/gcc-7-branch/gcc/ChangeLog
    branches/gcc-7-branch/gcc/config/rs6000/rs6000.md