[Bug target/96017] New: Powerpc suboptimal register spill in likely path

[Bug target/96017] New: Powerpc suboptimal register spill in likely path

[npiggin at gmail dot com]

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

            Bug ID: 96017
           Summary: Powerpc suboptimal register spill in likely path
           Product: gcc
           Version: 9.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: npiggin at gmail dot com
  Target Milestone: ---
            Target: powerpc64le-linux-gnu
             Build: gcc version 9.2.1 20190909 (Debian 9.2.1-8)

-- test.c --
extern int foo;
extern void slowpath(int *);

int test(int *val)
{
        int ret = foo;

        if (__builtin_expect(*val != 0, 0))
                slowpath(val);

        return ret;
}
--

Compiling with -O2 gives the following asm. It seems possible for the fast path
case to avoid the stack frame by using a volatile register to save the val
argument in case the slow path needs it (or alternatively to save the load from
'foo', as r31 is used now, but that requires an extra register move on a
critical path for the return value). This should be smaller and faster code
even for the slow path too.

        addis   r2,r12,0
        addi    r2,r2,0
        lwz     r9,0(r3)
        addis   r10,r2,0
        ld      r10,0(r10)
        std     r31,-8(r1)
        stdu    r1,-48(r1)
        lwa     r31,0(r10)
        cmpwi   r9,0
        bne     1f
        addi    r1,r1,48
        mr      r3,r31
        ld      r31,-8(r1)
        blr
        nop
        ori     r2,r2,0
1:      mflr    r0
        std     r0,64(r1)
        bl      slowpath
        nop
        ld      r0,64(r1)
        addi    r1,r1,48
        mr      r3,r31
        ld      r31,-8(r1)
        mtlr    r0
        blr

[Bug target/96017] Powerpc suboptimal register spill in likely path

[wschmidt at gcc dot gnu.org]

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

Bill Schmidt <wschmidt at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |segher at gcc dot gnu.org,
                   |                            |wschmidt at gcc dot gnu.org
   Target Milestone|---                         |9.4
              Build|gcc version 9.2.1 20190909  |
                   |(Debian 9.2.1-8)            |
           Keywords|                            |missed-optimization

--- Comment #1 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
Built with gcc version 9.2.1 20190909 (Debian 9.2.1-8) (moved from Build
field).

[Bug target/96017] Powerpc suboptimal register spill in likely path

[wschmidt at gcc dot gnu.org]

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

Bill Schmidt <wschmidt at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|9.4                         |11.0

[Bug target/96017] Powerpc suboptimal register spill in likely path

[wschmidt at gcc dot gnu.org]

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

--- Comment #2 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
Nick reports same behavior at -O3.

[Bug target/96017] Powerpc suboptimal register spill in likely path

[npiggin at gmail dot com]

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

--- Comment #3 from Nicholas Piggin <npiggin at gmail dot com> ---
This is possibly a more targeted and simpler test case for at least one of the
problems in bug 84443. I would like to re-test that case after this one is
solved if it's not an obvious duplicate.

[Bug target/96017] Powerpc suboptimal register spill in likely path

[bergner at gcc dot gnu.org]

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

--- Comment #4 from Peter Bergner <bergner at gcc dot gnu.org> ---
To be pedantic, "val" is assigned r3, the incoming arg reg.  The compiler
temporary that holds "*val" is assigned r9 which is a volatile register.  The
only non-volatile in use is r31 which was assigned to hold "ret".  Since "ret"
is live and used across both the fast and slow paths, it needs a non-volatile
register since it is live across the slowpath call.  The load of foo occurs
early because foo is a global and could be modified by the slowpath call.

What we want is for "ret" to be assigned a volatile register for the fast path
and only use a non-volatile for the slowpath.  That means we have to split the
live range before RA.  Shrink wrapping is too late here to help us.  We
basically want a function that looks like:

-- test.c --
extern int foo;
extern void slowpath(int *);

int test(int *val)
{
        int ret = foo;

        if (__builtin_expect(*val != 0, 0)) {
                int tmp = ret;
                slowpath(val);
                ret = tmp;
        }

        return ret;
}
--

Here "ret" doesn't live across the call so can be assigned a volatile register
and "tmp" which is live across the call and needs the non-volatile register is
only live in the slowpath, so shrink-wrapping can optimize it.

Doing this by hand doesn't help, because the early optimizers (not sure which
ones, didn't look) optimize those copies away and we end up with the same code,
so we'd want something that maybe adds those copies just before RA?

That said, rewriting the code like the following does seem to give good code on
the fast path:

-- test.c --
extern int foo;
extern void slowpath(int *);

int test(int *val)
{
        int ret = foo;

        if (__builtin_expect(*val != 0, 0)) {
                volatile int tmp = ret;
                slowpath(val);
                ret = tmp;
        }

        return ret;
}
--

0:      addis 2,12,.TOC.-.LCF0@ha
        addi 2,2,.TOC.-.LCF0@l
        .localentry     test,.-test
        lwz 10,0(3)
        addis 9,2,.LC0@toc@ha
        ld 9,.LC0@toc@l(9)
        lwa 9,0(9)
        cmpwi 0,10,0
        bne 0,.L11
        mr 3,9
        blr
        .p2align 4,,15
.L11:
        mflr 0
        std 0,16(1)
        stdu 1,-48(1)
        stw 9,32(1)
        bl slowpath
        nop
        lwa 9,32(1)
        addi 1,1,48
        ld 0,16(1)
        mr 3,9
        mtlr 0
        blr

[Bug target/96017] Powerpc suboptimal register spill in likely path

[segher at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2020-07-01
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

--- Comment #5 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Yes, or we should duplicate more of the function earlier (before RA),
for example the return tails.

Separate shrink wrapping does not duplicate code (it can easily
explode code size -- it will be interesting to see how much it can
help, if we restrict it somehow).  But it is r31 already before
shrink-wrapping -- we need some renaming / copying of registers (like
in Peter's code) to get rid of it.  In an example like this it is
quite useful, but in a lot of "real world" code there are no free
volatile registers to scarf up.  Or that was my impression anyway,
when I last looked at this.  Time to revisit it...

(The "ELFv1" code is just

.L.test:
        lwz 9,0(3)
        addis 10,2,.LC0@toc@ha
        ld 10,.LC0@toc@l(10)
        std 31,-8(1)
        stdu 1,-128(1)
        lwa 31,0(10)
        cmpwi 0,9,0
        bne 0,.L8
        addi 1,1,128
        mr 3,31
        ld 31,-8(1)
        blr
        .p2align 4,,15
.L8:
        mflr 0
        std 0,144(1)
        bl slowpath
        nop
        ld 0,144(1)
        addi 1,1,128
        mr 3,31
        ld 31,-8(1)
        mtlr 0
        blr

which feels simpler...  but it is kind of the same?

[Bug target/96017] Powerpc suboptimal register spill in likely path

[bergner at gcc dot gnu.org]

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

--- Comment #6 from Peter Bergner <bergner at gcc dot gnu.org> ---
(In reply to Segher Boessenkool from comment #5)
> But it is r31 already before
> shrink-wrapping -- we need some renaming / copying of registers (like
> in Peter's code) to get rid of it.  In an example like this it is
> quite useful, but in a lot of "real world" code there are no free
> volatile registers to scarf up.  Or that was my impression anyway,
> when I last looked at this.  Time to revisit it...

Right, that's why we need to add the copies before RA, so we don't have to look
for unused regs.  But we don't want to add the copies too early just for the
optimizer to remove them on us.  Like it did for my manually added copies.

There is ira.c:split_live_ranges_for_shrink_wrap().  I'll have a look to see
why it's not catching this test case. 


> (The "ELFv1" code is just
[snip]
> which feels simpler...  but it is kind of the same?

It does look better, but marginally so, since we still stack a frame and
save/restore r31 on the fast path.

[Bug target/96017] Powerpc suboptimal register spill in likely path

[bergner at gcc dot gnu.org]

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

--- Comment #7 from Peter Bergner <bergner at gcc dot gnu.org> ---
(In reply to Peter Bergner from comment #6)
> There is ira.c:split_live_ranges_for_shrink_wrap().  I'll have a look to see
> why it's not catching this test case. 

So it looks like it only splits pseudos that are set from a hard register (eg,
incoming param reg) and are live at a call.  I think we should be able to
expand on what we'll split.  I'll take a look at allowing other pseudos to be
split.

[Bug target/96017] Powerpc suboptimal register spill in likely path

[bergner at gcc dot gnu.org]

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

--- Comment #8 from Peter Bergner <bergner at gcc dot gnu.org> ---
Interesting, if I rewrite the test case so that foo is a parameter and not a
global var, then we get the code we want:

extern void slowpath(int *);

int
test (int *val, int foo)
{
  int ret = foo;

  if (__builtin_expect(*val != 0, 0))
    slowpath(val);

  return ret;
}

0:      addis 2,12,.TOC.-.LCF0@ha
        addi 2,2,.TOC.-.LCF0@l
        lwz 9,0(3)
        cmpwi 0,9,0
        bne 0,.L11
        mr 3,4
        blr
        .p2align 4,,15
.L11:
        mflr 0
        std 0,16(1)
        stdu 1,-48(1)
        std 4,32(1)
        bl slowpath
        nop
        ld 4,32(1)
        addi 1,1,48
        ld 0,16(1)
        mr 3,4
        mtlr 0
        blr

At first, I thought that split_live_ranges_for_shrink_wrap() split this nicely,
but what I found is that IRA assigned a volatile register to a pseudo that is
live across a call.  LRA then sees that is illegal and spills the pseudo which
ends up giving us what we want, but I don't like that.  IRA should never assign
a volatile register to a pseudo that is live across a call.  Yet another thing
to look into.

[Bug target/96017] Powerpc suboptimal register spill in likely path

[bergner at gcc dot gnu.org]

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

--- Comment #9 from Peter Bergner <bergner at gcc dot gnu.org> ---
(In reply to Peter Bergner from comment #8)
> At first, I thought that split_live_ranges_for_shrink_wrap() split this
> nicely, but what I found is that IRA assigned a volatile register to a
> pseudo that is live across a call.

...and that is because somehow flag_caller_saves is true.  I can't believe that
is a good idea in general.

[Bug target/96017] Powerpc suboptimal register spill in likely path

[segher at gcc dot gnu.org]

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

--- Comment #10 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Peter Bergner from comment #6)
> Right, that's why we need to add the copies before RA, so we don't have to
> look for unused regs.  But we don't want to add the copies too early just
> for the optimizer to remove them on us.

RA itself will want to use the same hard register most of the time, already.
And that *is* the best choice, except when you need to split the range later.

We would need to do (some of) shrink wrapping before RA, but that means we
need to change our abstraction of prologue/epilogue.  Not a bad idea probably,
but a lot of work.  Many targets do many special things in their own *logue
hooks (and patterns and code called from those) currently, it all would need
to be disentangled.

[Bug target/96017] Powerpc suboptimal register spill in likely path

[segher at gcc dot gnu.org]

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

--- Comment #11 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Peter Bergner from comment #9)
> (In reply to Peter Bergner from comment #8)
> > At first, I thought that split_live_ranges_for_shrink_wrap() split this
> > nicely, but what I found is that IRA assigned a volatile register to a
> > pseudo that is live across a call.
> 
> ...and that is because somehow flag_caller_saves is true.  I can't believe
> that is a good idea in general.

'-fcaller-saves'
     Enable allocation of values to registers that are clobbered by
     function calls, by emitting extra instructions to save and restore
     the registers around such calls.  Such allocation is done only when
     it seems to result in better code.

     This option is always enabled by default on certain machines,
     usually those which have no call-preserved registers to use
     instead.

     Enabled at levels '-O2', '-O3', '-Os'.

Note the "only when" line...  It sounds like (in spite of that promise) we
do it too aggressively currently?

[Bug target/96017] Powerpc suboptimal register spill in likely path

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|11.0                        |11.2

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 11.1 has been released, retargeting bugs to GCC 11.2.

[Bug target/96017] Powerpc suboptimal register spill in likely path

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|11.2                        |---

[Bug target/96017] Powerpc suboptimal register spill in likely path

[jskumari at gcc dot gnu.org]

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

--- Comment #13 from Surya Kumari Jangala <jskumari at gcc dot gnu.org> ---
With the patch at
https://gcc.gnu.org/pipermail/gcc-patches/2023-October/631849.html, the
testcase gets shrink wrapped. This is the assembly produced:

        addis 2,12,.TOC.-.LCF0@ha
        addi 2,2,.TOC.-.LCF0@l
        lwz %r10,0(%r3)
        addis %r9,%r2,.LC0@toc@ha
        ld %r9,.LC0@toc@l(%r9)
        cmpwi %cr0,%r10,0
        lwz %r9,0(%r9)
        bne %cr0,.L11
        extsw %r3,%r9
        blr
.L11:
        mflr %r0
        std %r0,16(%r1)
        stdu %r1,-48(%r1)
        stw %r9,32(%r1)
        bl slowpath
        nop
        lwz %r9,32(%r1)
        addi %r1,%r1,48
        ld %r0,16(%r1)
        extsw %r3,%r9
        mtlr %r0
        blr

[Bug target/96017] Powerpc suboptimal register spill in likely path

[jskumari at gcc dot gnu.org]

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

--- Comment #14 from Surya Kumari Jangala <jskumari at gcc dot gnu.org> ---
Instead of using a non-volatile register to hold the value of foo, a volatile
register (r9) is assigned to hold foo. This avoids setting up the stack frame
in the fast path.