[Bug target/103197] New: ppc inline expansion of memcpy/memmove should not use lxsibzx/stxsibx for a single byte

[Bug target/103197] New: ppc inline expansion of memcpy/memmove should not use lxsibzx/stxsibx for a single byte

[acsawdey at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103197

            Bug ID: 103197
           Summary: ppc inline expansion of memcpy/memmove should not use
                    lxsibzx/stxsibx for a single byte
           Product: gcc
           Version: 10.3.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: acsawdey at gcc dot gnu.org
  Target Milestone: ---

This got broken sometime in gcc 10 timeframe. For this test case:

#include <string.h>
void m(char *a, char *b)
{
  memcpy(a,b,9);
}

AT13 (gcc 9.3.1) produces:

m:
.LFB0:
        .cfi_startproc
        ld 10,0(4)
        lbz 9,8(4)
        std 10,0(3)
        stb 9,8(3)
        blr
        .long 0
        .byte 0,0,0,0,0,0,0,0
        .cfi_endproc

which is the expected code to copy 9 bytes.

AT14 (gcc 10.3.1), gcc 11, and current trunk all produce:

m:
.LFB0:
        .cfi_startproc
        addi 10,4,8
        ld 9,0(4)
        lxsibzx 0,0,10
        std 9,0(3)
        addi 9,3,8
        stxsibx 0,0,9
        blr
        .long 0
        .byte 0,0,0,0,0,0,0,0
        .cfi_endproc

which is really bad, mixing gpr and vsx. The inline expansion code in
expand_block_move() does not attempt to generate vsx code at all unless the
size is at least 16 bytes.

[Bug target/103197] ppc inline expansion of memcpy/memmove should not use lxsibzx/stxsibx for a single byte

[acsawdey at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103197

--- Comment #1 from acsawdey at gcc dot gnu.org ---
Looking at trunk, after expand we have this:

(note 5 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK)
(insn 2 5 3 2 (set (reg/v/f:DI 117 [ a ])
        (reg:DI 3 3 [ a ])) "bug2.c":3:1 -1
     (nil))
(insn 3 2 4 2 (set (reg/v/f:DI 118 [ b ])
        (reg:DI 4 4 [ b ])) "bug2.c":3:1 -1
     (nil))
(note 4 3 7 2 NOTE_INSN_FUNCTION_BEG)
(insn 7 4 9 2 (set (reg:DI 119)
        (mem:DI (reg/v/f:DI 118 [ b ]) [0 MEM <char[1:9]> [(void *)b_3(D)]+0 S8
A8])) "bug2.c":4:3 -1
     (nil))
(insn 9 7 8 2 (set (reg:QI 120)
        (mem:QI (plus:DI (reg/v/f:DI 118 [ b ])
                (const_int 8 [0x8])) [0 MEM <char[1:9]> [(void *)b_3(D)]+8 S1
A8])) "bug2.c":4:3 -1
     (nil))
(insn 8 9 10 2 (set (mem:DI (reg/v/f:DI 117 [ a ]) [0 MEM <char[1:9]> [(void
*)a_2(D)]+0 S8 A8])
        (reg:DI 119)) "bug2.c":4:3 -1
     (nil))
(insn 10 8 0 2 (set (mem:QI (plus:DI (reg/v/f:DI 117 [ a ])
                (const_int 8 [0x8])) [0 MEM <char[1:9]> [(void *)a_2(D)]+8 S1
A8])
        (reg:QI 120)) "bug2.c":4:3 -1
     (nil))

Which is the expected code, DI and QI loads/stores that should produce D-form
instructions.

But it looks like reload put the QI into hard reg 32 which is a fp reg:

(insn 9 17 8 2 (set (reg:QI 32 0 [orig:120 MEM <char[1:9]> [(void *)b_3(D)]+8 ]
[120])
        (mem:QI (reg:DI 10 10 [124]) [0 MEM <char[1:9]> [(void *)b_3(D)]+8 S1
A8])) "bug2.c":4:3 549 {*movqi_internal}

which leads to the lxsibzx/stxsibx on output.

[Bug target/103197] ppc inline expansion of memcpy/memmove should not use lxsibzx/stxsibx for a single byte

[acsawdey at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103197

--- Comment #2 from acsawdey at gcc dot gnu.org ---
>From the reload dump:

            0 Non input pseudo reload: reject++
            1 Non-pseudo reload: reject+=2
            1 Non input pseudo reload: reject++
          alt=0,overall=16,losers=2,rld_nregs=2
            0 Non input pseudo reload: reject++
          alt=1,overall=7,losers=1,rld_nregs=1
          alt=2,overall=6,losers=1,rld_nregs=0
[...]
         Choosing alt 2 in insn 9:  (0) wa  (1) Z {*movqi_internal}

The addressing for insn 9 is just reg+const so why did it think it would have
to reload one register for alt 1 (d-form) and 0 for alt 2 which is x-form?

[Bug target/103197] ppc inline expansion of memcpy/memmove should not use lxsibzx/stxsibx for a single byte

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103197

Segher Boessenkool <segher at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2021-11-12
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

--- Comment #3 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Also, it reloads the whole address into a register, instead of just the
offset.  The latter is much better to do (the offset can often be shared
with later insns, in this example already!)

But that aside.

Confirmed.  It only happens for power9 and up in my testing, does your
compiler default to that?

[Bug target/103197] ppc inline expansion of memcpy/memmove should not use lxsibzx/stxsibx for a single byte

[acsawdey at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103197

--- Comment #4 from acsawdey at gcc dot gnu.org ---
I was compiling with -mcpu=power9, yes:

/home2/sawdey/work/gcc/trunk/build/gcc/xgcc
-B/home2/sawdey/work/gcc/trunk/build/gcc -O3 -mcpu=power9 bug2.c

[Bug target/103197] ppc inline expansion of memcpy/memmove should not use lxsibzx/stxsibx for a single byte

[acsawdey at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103197

--- Comment #5 from acsawdey at gcc dot gnu.org ---
Bisection reveals that this starts with this commit:

20d70cd2719815d9ea853314775ae5787648ece5 is the first bad commit
commit 20d70cd2719815d9ea853314775ae5787648ece5
Author: Alan Modra <amodra@gmail.com>
Date:   Thu May 9 08:37:26 2019 +0930

    [RS6000] PR89271, gcc.target/powerpc/vsx-simode2.c

    This patch makes a number of corrections to rs6000_register_move_cost,
    adds a new register union class, GEN_OR_VSX_REGS, and adjusts insn
    alternative costs to suit.

[Bug target/103197] ppc inline expansion of memcpy/memmove should not use lxsibzx/stxsibx for a single byte

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103197

--- Comment #6 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Not only is this a missed-optimization, it also is a regression (in GCC 10
already).  It seems like the root cause here may be the same as in PR102169.

[Bug target/103197] ppc inline expansion of memcpy/memmove should not use lxsibzx/stxsibx for a single byte

[amodra at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103197

Alan Modra <amodra at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amodra at gmail dot com

--- Comment #7 from Alan Modra <amodra at gmail dot com> ---
(In reply to acsawdey from comment #2)
> From the reload dump:
No, that's too late.  In the ira dump gcc has already chosen a vsx reg.  See
r120.

Disposition:
    0:r117 l0     3    3:r118 l0     4    2:r119 l0     9    1:r120 l0    32
    4:r121 l0     3    5:r122 l0     4

[Bug target/103197] ppc inline expansion of memcpy/memmove should not use lxsibzx/stxsibx for a single byte

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103197

--- Comment #8 from Segher Boessenkool <segher at gcc dot gnu.org> ---
So it seems to think that all registers in the preferred class,
GEN_OR_VSX_REGS,
are the same cost?  They very much are not :-(

[Bug target/103197] ppc inline expansion of memcpy/memmove should not use lxsibzx/stxsibx for a single byte

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103197

--- Comment #9 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Created attachment 52131
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52131&action=edit
Proposed patch

[Bug target/103197] ppc inline expansion of memcpy/memmove should not use lxsibzx/stxsibx for a single byte

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103197

Segher Boessenkool <segher at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |npiggin at gmail dot com

--- Comment #10 from Segher Boessenkool <segher at gcc dot gnu.org> ---
*** Bug 102169 has been marked as a duplicate of this bug. ***

[Bug target/103197] ppc inline expansion of memcpy/memmove should not use lxsibzx/stxsibx for a single byte

[guihaoc at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103197

HaoChen Gui <guihaoc at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |guihaoc at gcc dot gnu.org

--- Comment #11 from HaoChen Gui <guihaoc at gcc dot gnu.org> ---
Segher,
  Will you commit your patch in stage4? Several issues are supposed to be fixed
by your patch. Thanks.

[Bug target/103197] ppc inline expansion of memcpy/memmove should not use lxsibzx/stxsibx for a single byte

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103197

--- Comment #12 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to HaoChen Gui from comment #11)
> Segher,
>   Will you commit your patch in stage4? Several issues are supposed to be
> fixed by your patch. Thanks.

Yes, of course, but there have been complications.  In particular this had to
be *thoroughly* tested.

[Bug target/103197] ppc inline expansion of memcpy/memmove should not use lxsibzx/stxsibx for a single byte

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103197

--- Comment #13 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Segher Boessenkool <segher@gcc.gnu.org>:

https://gcc.gnu.org/g:26fa464f42622c60d6929720dd37143a21054ede

commit r12-8221-g26fa464f42622c60d6929720dd37143a21054ede
Author: Segher Boessenkool <segher@kernel.crashing.org>
Date:   Sun Jan 2 14:08:35 2022 +0000

    rs6000: Disparage lfiwzx and similar

    RA now chooses GEN_OR_VSX_REGS in most cases.  This is great in most
    cases, but we often (or always?) use {l,st}{f,xs}iwzx now, which is
    problematic because the integer load and store insns can use cheaper
    addressing modes.  We can fix that by putting a small penalty on the
    instruction alternatives for those.

    2022-04-21  Segher Boessenkool  <segher@kernel.crashing.org>

            PR target/103197
            PR target/102146
            * config/rs6000/rs6000.md (zero_extendqi<mode>2 for EXTQI):
Disparage
            the "Z" alternatives in {l,st}{f,xs}iwzx.
            (zero_extendhi<mode>2 for EXTHI): Ditto.
            (zero_extendsi<mode>2 for EXTSI): Ditto.
            (*movsi_internal1): Ditto.
            (*mov<mode>_internal1 for QHI): Ditto.
            (movsd_hardfloat): Ditto.

[Bug target/103197] ppc inline expansion of memcpy/memmove should not use lxsibzx/stxsibx for a single byte

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103197

--- Comment #14 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Segher Boessenkool <segher@gcc.gnu.org>:

https://gcc.gnu.org/g:748d46cd049c89a799f99f14547267ebae915af6

commit r12-8222-g748d46cd049c89a799f99f14547267ebae915af6
Author: Segher Boessenkool <segher@kernel.crashing.org>
Date:   Thu Apr 21 18:35:32 2022 +0000

    rs6000/testsuite: xfail bswap-brw.c

    This testcase does not generate anywhere near optimal code for 32-bit
    code.  For p10 it actually now fails this testcase, after the previous
    patch.  Let's xfail it.

    2022-04-21  Segher Boessenkool  <segher@kernel.crashing.org>

    gcc/testsuite/
            PR target/103197
            PR target/102146
            * gcc.target/powerpc/bswap-brw.c: Add xfail on scan-assembler for
-m32.

[Bug target/103197] [10/11] ppc inline expansion of memcpy/memmove should not use lxsibzx/stxsibx for a single byte

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103197

--- Comment #15 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Segher Boessenkool
<segher@gcc.gnu.org>:

https://gcc.gnu.org/g:2eb21e7349cda2885438463f045f6729a47039e8

commit r11-10207-g2eb21e7349cda2885438463f045f6729a47039e8
Author: Segher Boessenkool <segher@kernel.crashing.org>
Date:   Sun Jan 2 14:08:35 2022 +0000

    rs6000: Disparage lfiwzx and similar

    RA now chooses GEN_OR_VSX_REGS in most cases.  This is great in most
    cases, but we often (or always?) use {l,st}{f,xs}iwzx now, which is
    problematic because the integer load and store insns can use cheaper
    addressing modes.  We can fix that by putting a small penalty on the
    instruction alternatives for those.

    2022-04-21  Segher Boessenkool  <segher@kernel.crashing.org>

            PR target/103197
            PR target/102146
            * config/rs6000/rs6000.md (zero_extendqi<mode>2 for EXTQI):
Disparage
            the "Z" alternatives in {l,st}{f,xs}iwzx.
            (zero_extendhi<mode>2 for EXTHI): Ditto.
            (zero_extendsi<mode>2 for EXTSI): Ditto.
            (*movsi_internal1): Ditto.
            (*mov<mode>_internal1 for QHI): Ditto.
            (movsd_hardfloat): Ditto.

    (cherry picked from commit 26fa464f42622c60d6929720dd37143a21054ede)

[Bug target/103197] [10/11] ppc inline expansion of memcpy/memmove should not use lxsibzx/stxsibx for a single byte

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103197

--- Comment #16 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Segher Boessenkool
<segher@gcc.gnu.org>:

https://gcc.gnu.org/g:72cf56c7cfefaf1b074bb70e42890cf1191c46a1

commit r11-10208-g72cf56c7cfefaf1b074bb70e42890cf1191c46a1
Author: Segher Boessenkool <segher@kernel.crashing.org>
Date:   Thu Apr 21 18:35:32 2022 +0000

    rs6000/testsuite: xfail bswap-brw.c

    This testcase does not generate anywhere near optimal code for 32-bit
    code.  For p10 it actually now fails this testcase, after the previous
    patch.  Let's xfail it.

    2022-04-21  Segher Boessenkool  <segher@kernel.crashing.org>

    gcc/testsuite/
            PR target/103197
            PR target/102146
            * gcc.target/powerpc/bswap-brw.c: Add xfail on scan-assembler for
-m32.

    (cherry picked from commit 748d46cd049c89a799f99f14547267ebae915af6)

[Bug target/103197] [10/11] ppc inline expansion of memcpy/memmove should not use lxsibzx/stxsibx for a single byte

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103197

--- Comment #17 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Segher Boessenkool
<segher@gcc.gnu.org>:

https://gcc.gnu.org/g:d99d74d8b1b517784e3b05b271b977eb6603121f

commit r10-10948-gd99d74d8b1b517784e3b05b271b977eb6603121f
Author: Segher Boessenkool <segher@kernel.crashing.org>
Date:   Sun Jan 2 14:08:35 2022 +0000

    rs6000: Disparage lfiwzx and similar

    RA now chooses GEN_OR_VSX_REGS in most cases.  This is great in most
    cases, but we often (or always?) use {l,st}{f,xs}iwzx now, which is
    problematic because the integer load and store insns can use cheaper
    addressing modes.  We can fix that by putting a small penalty on the
    instruction alternatives for those.

    2022-04-21  Segher Boessenkool  <segher@kernel.crashing.org>

            PR target/103197
            PR target/102146
            * config/rs6000/rs6000.md (zero_extendqi<mode>2 for EXTQI):
Disparage
            the "Z" alternatives in {l,st}{f,xs}iwzx.
            (zero_extendhi<mode>2 for EXTHI): Ditto.
            (zero_extendsi<mode>2 for EXTSI): Ditto.
            (*movsi_internal1): Ditto.
            (*mov<mode>_internal1 for QHI): Ditto.
            (movsd_hardfloat): Ditto.

    (cherry picked from commit 26fa464f42622c60d6929720dd37143a21054ede)

[Bug target/103197] [10/11] ppc inline expansion of memcpy/memmove should not use lxsibzx/stxsibx for a single byte

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103197

Segher Boessenkool <segher at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #18 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Fixed everywhere.