[Bug target/54602] New: [SH] Register pop insn not put in rts delay slot

[Bug target/54602] New: [SH] Register pop insn not put in rts delay slot

[olegendo at gcc dot gnu.org]

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

             Bug #: 54602
           Summary: [SH] Register pop insn not put in rts delay slot
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: enhancement
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: olegendo@gcc.gnu.org
            Target: sh*-*-*


The following case:

int test01 (int a, int b);

int test00 (int a, int b, int c, int d)
{
  return test01 (a, b) + c;
}

Compiled with -O2 -m2:
        mov.l    .L3,r0          ! 9    movsi_i/1    [length = 2]
        mov.l   r8,@-r15        ! 26    movsi_i/7    [length = 2]
        sts.l   pr,@-r15        ! 27    movsi_i/9    [length = 2]
        jsr     @r0             ! 12    call_valuei    [length = 2]
        mov     r6,r8           ! 4    movsi_i/2    [length = 2]
        add     r8,r0           ! 14    *addsi3_compact    [length = 2]
        lds.l   @r15+,pr        ! 33    *movsi_pop/3    [length = 2]
        mov.l   @r15+,r8        ! 34    *movsi_pop/1    [length = 2]
        rts
        nop                     ! 36    *return_i    [length = 4]

Compiled with -O2 -m2a:
        mov.l   .L3,r0          ! 9    movsi_ie/1    [length = 2]
        mov.l   r8,@-r15        ! 30    movsi_ie/9    [length = 4]
        sts.l   pr,@-r15        ! 31    movsi_ie/11    [length = 2]
        jsr     @r0             ! 12    call_valuei    [length = 2]
        mov     r6,r8           ! 4    movsi_ie/2    [length = 2]
        add     r8,r0           ! 14    *addsi3_compact    [length = 2]
        lds.l   @r15+,pr        ! 37    *movsi_pop/3    [length = 2]
        mov.l   @r15+,r8        ! 38    *movsi_pop/1    [length = 2]
        rts/n                   ! 40    *return_i    [length = 4]

The pop insn of r8 is not placed in the delay slot of rts without any obvious
reason.  This happens only for -m1 and -m2*.

[Bug target/54602] [SH] Register pop insn not put in rts delay slot

[olegendo at gcc dot gnu.org]

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

Oleg Endo <olegendo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-10-09
     Ever Confirmed|0                           |1

--- Comment #1 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-10-09 22:52:19 UTC ---
The problem is the "*movsi_pop" insn.  It prevents delay-slot stuffing of pop
insns on everything < SH3.  This was probably added to avoid pop insns in
interrupt handler functions, which return with 'rte' instead of 'rts', see PR
53689.
Adding "&& current_function_interrupt" to the condition of the "*movsi_pop"
insn fixes the problem, while maintaining correct code for interrupt handler
functions.

There is also the following in sh.md:

(define_delay
  (eq_attr "type" "return")
  [(and (eq_attr "in_delay_slot" "yes")
    (ior (and (eq_attr "interrupt_function" "no")
          (eq_attr "type" "!pload,prset"))
         (and (eq_attr "interrupt_function" "yes")
          (ior
           (not (match_test "TARGET_SH3"))
           (eq_attr "hit_stack" "no")
           (eq_attr "banked" "no"))))) (nil) (nil)])

which should actually prevent normal movsi pop insn from slipping into the
delay slot.  I've tapped the "hit_stack" test and it is not invoked for
interrupt functions for some reason ....

[Bug target/54602] [SH] Register pop insn not put in rts delay slot

[olegendo at gcc dot gnu.org]

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

--- Comment #2 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-10-09 22:57:40 UTC ---
(In reply to comment #1)
> The problem is the "*movsi_pop" insn.  It prevents delay-slot stuffing of pop
> insns on everything < SH3.  This was probably added to avoid pop insns in
> interrupt handler functions, which return with 'rte' instead of 'rts', see PR
> 53689.
> Adding "&& current_function_interrupt" to the condition of the "*movsi_pop"
> insn fixes the problem, while maintaining correct code for interrupt handler
> functions.
> 
> There is also the following in sh.md:
> 
> (define_delay
>   (eq_attr "type" "return")
>   [(and (eq_attr "in_delay_slot" "yes")
>     (ior (and (eq_attr "interrupt_function" "no")
>           (eq_attr "type" "!pload,prset"))
>          (and (eq_attr "interrupt_function" "yes")
>           (ior
>            (not (match_test "TARGET_SH3"))
>            (eq_attr "hit_stack" "no")
>            (eq_attr "banked" "no"))))) (nil) (nil)])
> 
> which should actually prevent normal movsi pop insn from slipping into the
> delay slot.  I've tapped the "hit_stack" test and it is not invoked for
> interrupt functions for some reason ....

... because this define_delay applies for return insns, not for mem loads. 
D'uh. :T

[Bug target/54602] [SH] Register pop insn not put in rts delay slot

[olegendo at gcc dot gnu.org]

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

Oleg Endo <olegendo at gcc dot gnu.org> changed:

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

--- Comment #3 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-10-10 00:43:39 UTC ---
I was right after all.  The checks in (define_delay (eq_attr "type" "return") 
somehow got lost in parenthesis and the "*movsi_pop" insn is not required after
all.  The following fixes the issue:

Index: gcc/config/sh/sh.md
===================================================================
--- gcc/config/sh/sh.md    (revision 192200)
+++ gcc/config/sh/sh.md    (working copy)
@@ -541,22 +541,22 @@
   (eq_attr "needs_delay_slot" "yes")
   [(eq_attr "in_delay_slot" "yes") (nil) (nil)])

+;; Since a normal return (rts) implicitly uses the PR register,
+;; we can't allow PR register loads in an rts delay slot.
 ;; On the SH and SH2, the rte instruction reads the return pc from the stack,
 ;; and thus we can't put a pop instruction in its delay slot.
 ;; On the SH3 and SH4, the rte instruction does not use the stack, so a pop
-;; instruction can go in the delay slot.
-;; Since a normal return (rts) implicitly uses the PR register,
-;; we can't allow PR register loads in an rts delay slot.
+;; instruction can go in the delay slot, unless it references a banked
+;; register (the register bank is switched by rte).
 (define_delay
   (eq_attr "type" "return")
   [(and (eq_attr "in_delay_slot" "yes")
     (ior (and (eq_attr "interrupt_function" "no")
           (eq_attr "type" "!pload,prset"))
          (and (eq_attr "interrupt_function" "yes")
-          (ior
-           (not (match_test "TARGET_SH3"))
-           (eq_attr "hit_stack" "no")
-           (eq_attr "banked" "no"))))) (nil) (nil)])
+          (ior (match_test "TARGET_SH3") (eq_attr "hit_stack" "no"))
+          (eq_attr "banked" "no"))))
+   (nil) (nil)])

 ;; Since a call implicitly uses the PR register, we can't allow
 ;; a PR register store in a jsr delay slot.
@@ -6186,21 +6186,6 @@
   emit_insn (gen_ashlsi3 (operands[5], operands[0], operands[2]));
 })

-;; Define additional pop for SH1 and SH2 so it does not get 
-;; placed in the delay slot.
-(define_insn "*movsi_pop"
-  [(set (match_operand:SI 0 "register_operand" "=r,x,l")
-        (match_operand:SI 1 "sh_no_delay_pop_operand" ">,>,>"))]
-  "(TARGET_SH1 || TARGET_SH2E || TARGET_SH2A)
-   && ! TARGET_SH3"
-  "@
-    mov.l    %1,%0
-    lds.l    %1,%0
-    lds.l    %1,%0"
-  [(set_attr "type" "load_si,mem_mac,pload")
-   (set_attr "length" "2,2,2")
-   (set_attr "in_delay_slot" "no,no,no")])
-
 ;; t/r must come after r/r, lest reload will try to reload stuff like
 ;; (set (subreg:SI (mem:QI (plus:SI (reg:SI SP_REG) (const_int 12)) 0) 0)
 ;; (made from (set (subreg:SI (reg:QI ###) 0) ) into T.


Kaz, could you please also have a pre-look at this?  I might be missing
something...

Also, I've noticed that on SH4 (which has banked regs R0..R7) the banked regs
are also saved / restored in an interrupt function.  This actually defeats the
purpose of the R0..R7 register bank.  Maybe some historic reason, or just
accident?

[Bug target/54602] [SH] Register pop insn not put in rts delay slot

[kkojima at gcc dot gnu.org]

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

--- Comment #4 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2012-10-10 22:44:17 UTC ---
(In reply to comment #3)
> Kaz, could you please also have a pre-look at this?  I might be missing
> something...

Looks reasonable to me, though I also might be missing something.

> Also, I've noticed that on SH4 (which has banked regs R0..R7) the banked regs
> are also saved / restored in an interrupt function.  This actually defeats the
> purpose of the R0..R7 register bank.  Maybe some historic reason, or just
> accident?

I don't know the history about it.  I can only imagine that some
system could assume some banked regs will be not clobbered with
their exception handler and will be used like as normal registers.
A new -m option which controls the behavior of which default
is not to save/restore the banked regs?

[Bug target/54602] [SH] Register pop insn not put in rts delay slot

[olegendo at gcc dot gnu.org]

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

--- Comment #5 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-10-10 23:24:07 UTC ---
(In reply to comment #4)
> 
> I don't know the history about it.  I can only imagine that some
> system could assume some banked regs will be not clobbered with
> their exception handler and will be used like as normal registers.
> A new -m option which controls the behavior of which default
> is not to save/restore the banked regs?

Oh well, why not ;)
But first I'd like to think this through.  I'll open a new PR for it later.

[Bug target/54602] [SH] Register pop insn not put in rts delay slot

[olegendo at gcc dot gnu.org]

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

--- Comment #6 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-10-11 13:48:04 UTC ---
(In reply to comment #5)
> (In reply to comment #4)
> > 
> > I don't know the history about it.  I can only imagine that some
> > system could assume some banked regs will be not clobbered with
> > their exception handler and will be used like as normal registers.
> > A new -m option which controls the behavior of which default
> > is not to save/restore the banked regs?
> 
> Oh well, why not ;)
> But first I'd like to think this through.  I'll open a new PR for it later.

Turns out, there's already something there: 'nosave_low_regs' function
attribute, which is documented only in the source code:

sh.c (line ~9300):
   nosave_low_regs - don't save r0..r7 in an interrupt handler.
     This is useful on the SH3 and upwards,
     which has a separate set of low regs for User and Supervisor modes.
     This should only be used for the lowest level of interrupts.  Higher
levels
     of interrupts must save the registers in case they themselves are
     interrupted.

I will improve the documentation regarding this.

[Bug target/54602] [SH] Register pop insn not put in rts delay slot

[olegendo at gcc dot gnu.org]

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

--- Comment #7 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-10-12 23:22:52 UTC ---
Author: olegendo
Date: Fri Oct 12 23:22:48 2012
New Revision: 192417

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=192417
Log:
    PR target/54602
    * config/sh/sh.md: Correct define_delay for return insns.
    (*movsi_pop): Delete.

    PR target/54602
    * gcc.target/sh/pr54602-1.c: New.
    * gcc.target/sh/pr54602-2.c: New.
    * gcc.target/sh/pr54602-3.c: New.
    * gcc.target/sh/pr54602-4.c: New.


Added:
    trunk/gcc/testsuite/gcc.target/sh/pr54602-1.c
    trunk/gcc/testsuite/gcc.target/sh/pr54602-2.c
    trunk/gcc/testsuite/gcc.target/sh/pr54602-3.c
    trunk/gcc/testsuite/gcc.target/sh/pr54602-4.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/sh/sh.md
    trunk/gcc/testsuite/ChangeLog

[Bug target/54602] [SH] Register pop insn not put in rts delay slot

[olegendo at gcc dot gnu.org]

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

Oleg Endo <olegendo at gcc dot gnu.org> changed:

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

--- Comment #8 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-10-30 01:26:16 UTC ---
Fixed in 4.8.