[Bug target/49881] New: [AVR] Inefficient stack manipulation around calls

[Bug target/49881] New: [AVR] Inefficient stack manipulation around calls

[rth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49881

           Summary: [AVR] Inefficient stack manipulation around calls
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: enhancement
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: rth@gcc.gnu.org


Created attachment 24848
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24848
Hack to set ACCUMULATE_OUTGOING_ARGS

While looking at PR49864 I noticed some awful code.

First, the argument setup code doesn't use push:

        rcall .
        rcall .
        rcall .
        in r30,__SP_L__
        in r31,__SP_H__
        adiw r30,1
        in r26,__SP_L__
        in r27,__SP_H__
        adiw r26,1+1
        st X,r15
        st -X,r14
        sbiw r26,1
        ld r24,Y
        ldd r25,Y+1
        std Z+3,r25
        std Z+2,r24
        lds r24,a1
        lds r25,a1+1
        std Z+5,r25
        std Z+4,r24
        rcall printf

vs a hand-written

        lds r24,a1
        lds r25,a1+1
        push r25
        push r24
        ld r24,Y
        ldd r25,Y+1
        push r25
        push r24
        push r15
        push r14
        rcall printf

If that can be fixed, then the 9 insns to pop the stack afterward,

        in r18,__SP_L__
        in r19,__SP_H__
        subi r18,lo8(-(6))
        sbci r19,hi8(-(6))
        in __tmp_reg__,__SREG__
        cli 
        out __SP_H__,r19
        out __SREG__,__tmp_reg__
        out __SP_L__,r18

might be ok.  If that's tricky, consider switching the port to use
ACCUMULATE_OUTGOING_ARGS.  A quick hack (attached) showed a nice
to this test case:

        in r30,__SP_L__
        in r31,__SP_H__
        std Z+2,r13
        std Z+1,r12
        mov r30,r16
        mov r31,r17
        ld r24,Z
        ldd r25,Z+1
        in r30,__SP_L__
        in r31,__SP_H__
        std Z+4,r25
        std Z+3,r24
        lds r24,a1
        lds r25,a1+1
        std Z+6,r25
        std Z+5,r24
        rcall printf

With total output

   text       data        bss        dec        hex    filename
   2311         32          0       2343        927    z-before.o
   1805         32          0       1837        72d    z-after.o

Even if you do manage to fix the push problem, it might be worthwhile
to add -maccumulate-outgoing-args, like for the i386 port.  That would
give a user the option of changing the logic to suit their source.

[Bug target/49881] [AVR] Inefficient stack manipulation around calls

[eric.weddington at atmel dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49881

--- Comment #1 from Eric Weddington <eric.weddington at atmel dot com> 2011-07-27 23:13:31 UTC ---
(In reply to comment #0)
> Created attachment 24848 [details]
> Hack to set ACCUMULATE_OUTGOING_ARGS
> 
> While looking at PR49864 I noticed some awful code.
> 
> First, the argument setup code doesn't use push:
> 
>         rcall .
>         rcall .
>         rcall .

Please note that the rcall instruction has a side effect of subtracting 2 bytes
from the SP (or 3 bytes if the avr device has a 22-bit PC). This side effect is
used in doing the "rcall ." instructions as an optimization to avoid doing 2
pushes (the rcall and push instructions are the same size).

But I agree that the code looks bad, and you show an improvement.

Where is this test case that you're referring to? I can only find your patch at
this PR, and only a patch at PR49864.

Thanks,
Eric Weddington

[Bug target/49881] [AVR] Inefficient stack manipulation around calls

[rth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49881

Richard Henderson <rth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011.07.28 00:04:56
     Ever Confirmed|0                           |1

--- Comment #2 from Richard Henderson <rth at gcc dot gnu.org> 2011-07-28 00:04:56 UTC ---
The source for this bug, and the source for PR49864 is

  gcc.c-torture/unsorted/gen_tst.c,  -O3 -g

[Bug target/49881] [AVR] Inefficient stack manipulation around calls

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49881

Georg-Johann Lay <gjl at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization

--- Comment #3 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2011-07-28 09:26:06 UTC ---
(In reply to comment #0)
>         in r30,__SP_L__
>         in r31,__SP_H__
>         adiw r30,1
>         in r26,__SP_L__
>         in r27,__SP_H__
>         adiw r26,1+1
>         st X,r15
>         st -X,r14
>         sbiw r26,1

This part might be related to PR46278 (fake X addressing).

[Bug target/49881] [AVR] Inefficient stack manipulation around calls

[rth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49881

Richard Henderson <rth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #24848|0                           |1
        is obsolete|                            |

--- Comment #4 from Richard Henderson <rth at gcc dot gnu.org> 2011-07-28 15:43:49 UTC ---
Created attachment 24857
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24857
patch to generate pushes

Really just a simple matter of defining the right stuff.
On the aforementioned test case we now get

        lds r24,a1+1
        push r24
        lds r24,a1
        push r24
        ldd r24,Y+1
        push r24
        ld r24,Y
        push r24
        push r15
        push r14
        rcall printf

and

   text       data        bss        dec        hex    filename
   1487         32          0       1519        5ef    z.o

which is even better than the hacky A_O_A patch, and
nearly 1K smaller than the unpatched output.

Would one of you be so kind as to do a full test run?
I don't have avr-libc et al set up on this machine...

[Bug target/49881] [AVR] Inefficient stack manipulation around calls

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49881

Georg-Johann Lay <gjl at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |49864

--- Comment #5 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2011-07-28 16:10:35 UTC ---
(In reply to comment #4)
> Created attachment 24857 [details]
> patch to generate pushes
> 
> [...]
>
> Would one of you be so kind as to do a full test run?
> I don't have avr-libc et al set up on this machine...

Regenerating avr-libc (with your patch atop r176818) fails with PR49864 and the
exact line number from there.  Do you need more food, or is the test case in
PR49864 sufficient to fix that PR?

[Bug target/49881] [AVR] Inefficient stack manipulation around calls

[rth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49881

--- Comment #6 from Richard Henderson <rth at gcc dot gnu.org> 2011-07-28 16:14:46 UTC ---
(In reply to comment #5)
> Regenerating avr-libc (with your patch atop r176818) fails with PR49864 and the
> exact line number from there.  Do you need more food, or is the test case in
> PR49864 sufficient to fix that PR?

Bother.  Well, you can take the patch in PR49864 for now.
I'll keep working on fixing that one properly...

[Bug target/49881] [AVR] Inefficient stack manipulation around calls

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49881

--- Comment #7 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2011-07-29 14:39:31 UTC ---
(In reply to comment #4)

> Would one of you be so kind as to do a full test run?
> I don't have avr-libc et al set up on this machine...

Just running tests without regenerating libs:

gcc.c-torture/compile/20030903-1.c:23:1: error: unrecognizable insn:

(insn 43 42 44 3 (set (mem:HI (post_dec:HI (reg/f:HI 32 __SP_L__)) [0 S2 A8])

        (reg:HI 68))
/mnt/nfs/home/georg/gnu/gcc.gnu.org/trunk/gcc/testsuite/gcc.c-torture/compile/20030903-1.c:15
-1

     (nil))


FAIL: gcc.c-torture/compile/20030903-1.c  -O0
FAIL: gcc.c-torture/compile/20030903-1.c  -O1
FAIL: gcc.c-torture/compile/20030903-1.c  -O2
FAIL: gcc.c-torture/compile/20030903-1.c  -O3 -fomit-frame-pointer
FAIL: gcc.c-torture/compile/20030903-1.c  -O3 -g
FAIL: gcc.c-torture/compile/20030903-1.c  -Os

gcc.c-torture/execute/complex-7.c:56:1: error: unrecognizable insn:

(insn 15 14 16 3 (set (mem:SF (post_dec:HI (reg/f:HI 32 __SP_L__)) [0 S4 A8])

        (reg:SF 43 [ f5.0+4 ]))

FAIL: gcc.c-torture/execute/complex-7.c compilation,  -O0
FAIL: gcc.c-torture/execute/complex-7.c compilation,  -O1
FAIL: gcc.c-torture/execute/complex-7.c compilation,  -O2
FAIL: gcc.c-torture/execute/complex-7.c compilation,  -O3 -fomit-frame-pointer
FAIL: gcc.c-torture/execute/complex-7.c compilation,  -O3 -g
FAIL: gcc.c-torture/execute/complex-7.c compilation,  -Os

There are many other FAILs that go into the same direction.

-------------------------------------------------

The good news is that the following known FAILs, aka. PR49629, pass with your
patch:

gcc.c-torture/compile/pr34448.c: In function 'build_attr_cert':

gcc.c-torture/compile/pr34448.c:31:1: internal compiler error: in
df_refs_verify, at df-scan.c:4268

Please submit a full bug report,

with preprocessed source if appropriate.

See <http://gcc.gnu.org/bugs.html> for instructions.


FAIL: gcc.c-torture/compile/pr34448.c  -O2  (internal compiler error)
FAIL: gcc.c-torture/compile/pr34448.c  -O2  (test for excess errors)
FAIL: gcc.c-torture/compile/pr34448.c  -O3 -fomit-frame-pointer  (internal
compiler error)
FAIL: gcc.c-torture/compile/pr34448.c  -O3 -fomit-frame-pointer  (test for
excess errors)
FAIL: gcc.c-torture/compile/pr34448.c  -O3 -g  (internal compiler error)
FAIL: gcc.c-torture/compile/pr34448.c  -O3 -g  (test for excess errors)

FAIL: gcc.c-torture/compile/pr39928-1.c  -O2  (internal compiler error)
FAIL: gcc.c-torture/compile/pr39928-1.c  -O2  (test for excess errors)
FAIL: gcc.c-torture/compile/pr39928-1.c  -O3 -fomit-frame-pointer  (internal
compiler error)
FAIL: gcc.c-torture/compile/pr39928-1.c  -O3 -fomit-frame-pointer  (test for
excess errors)
FAIL: gcc.c-torture/compile/pr39928-1.c  -O3 -g  (internal compiler error)
FAIL: gcc.c-torture/compile/pr39928-1.c  -O3 -g  (test for excess errors)
P

[Bug target/49881] [AVR] Inefficient stack manipulation around calls

[rth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49881

--- Comment #8 from Richard Henderson <rth at gcc dot gnu.org> 2011-08-01 19:35:46 UTC ---
Author: rth
Date: Mon Aug  1 19:35:43 2011
New Revision: 177071

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=177071
Log:
PR target/49881
        * config/avr/avr.h (PUSH_ROUNDING): New.
        * config/avr/avr.md (pushqi1): Rename from *pushqi.
        (*pushhi, *pushsi, *pushsf): Remove.
        (MPUSH): New mode iterator.
        (push<MPUSH>1): New expander.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/avr/avr.h
    trunk/gcc/config/avr/avr.md

[Bug target/49881] [AVR] Inefficient stack manipulation around calls

[rth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49881

--- Comment #9 from Richard Henderson <rth at gcc dot gnu.org> 2011-08-02 17:38:22 UTC ---
Author: rth
Date: Tue Aug  2 17:38:16 2011
New Revision: 177196

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=177196
Log:
PR target/49881
        * config/avr/avr.md (push<MPUSH>1): Don't constrain the operand.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/avr/avr.md

[Bug target/49881] [AVR] Inefficient stack manipulation around calls

[rth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49881

Richard Henderson <rth at gcc dot gnu.org> changed:

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

--- Comment #10 from Richard Henderson <rth at gcc dot gnu.org> 2011-08-02 22:22:10 UTC ---
I think we can consider this fixed.
Certainly we're doing much better for this specific case.

[Bug target/49881] [AVR] Inefficient stack manipulation around calls

[rth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49881

Richard Henderson <rth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.7.0