* [Bug target/54602] [SH] Register pop insn not put in rts delay slot
2012-09-16 15:26 [Bug target/54602] New: [SH] Register pop insn not put in rts delay slot olegendo at gcc dot gnu.org
@ 2012-10-09 22:52 ` olegendo at gcc dot gnu.org
2012-10-09 22:58 ` olegendo at gcc dot gnu.org
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: olegendo at gcc dot gnu.org @ 2012-10-09 22:52 UTC (permalink / raw)
To: gcc-bugs
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 ....
^ permalink raw reply [flat|nested] 9+ messages in thread* [Bug target/54602] [SH] Register pop insn not put in rts delay slot
2012-09-16 15:26 [Bug target/54602] New: [SH] Register pop insn not put in rts delay slot olegendo at gcc dot gnu.org
2012-10-09 22:52 ` [Bug target/54602] " olegendo at gcc dot gnu.org
@ 2012-10-09 22:58 ` olegendo at gcc dot gnu.org
2012-10-10 0:44 ` olegendo at gcc dot gnu.org
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: olegendo at gcc dot gnu.org @ 2012-10-09 22:58 UTC (permalink / raw)
To: gcc-bugs
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [Bug target/54602] [SH] Register pop insn not put in rts delay slot
2012-09-16 15:26 [Bug target/54602] New: [SH] Register pop insn not put in rts delay slot olegendo at gcc dot gnu.org
2012-10-09 22:52 ` [Bug target/54602] " olegendo at gcc dot gnu.org
2012-10-09 22:58 ` olegendo at gcc dot gnu.org
@ 2012-10-10 0:44 ` olegendo at gcc dot gnu.org
2012-10-10 22:44 ` kkojima at gcc dot gnu.org
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: olegendo at gcc dot gnu.org @ 2012-10-10 0:44 UTC (permalink / raw)
To: gcc-bugs
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?
^ permalink raw reply [flat|nested] 9+ messages in thread* [Bug target/54602] [SH] Register pop insn not put in rts delay slot
2012-09-16 15:26 [Bug target/54602] New: [SH] Register pop insn not put in rts delay slot olegendo at gcc dot gnu.org
` (2 preceding siblings ...)
2012-10-10 0:44 ` olegendo at gcc dot gnu.org
@ 2012-10-10 22:44 ` kkojima at gcc dot gnu.org
2012-10-10 23:24 ` olegendo at gcc dot gnu.org
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: kkojima at gcc dot gnu.org @ 2012-10-10 22:44 UTC (permalink / raw)
To: gcc-bugs
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?
^ permalink raw reply [flat|nested] 9+ messages in thread* [Bug target/54602] [SH] Register pop insn not put in rts delay slot
2012-09-16 15:26 [Bug target/54602] New: [SH] Register pop insn not put in rts delay slot olegendo at gcc dot gnu.org
` (3 preceding siblings ...)
2012-10-10 22:44 ` kkojima at gcc dot gnu.org
@ 2012-10-10 23:24 ` olegendo at gcc dot gnu.org
2012-10-11 13:48 ` olegendo at gcc dot gnu.org
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: olegendo at gcc dot gnu.org @ 2012-10-10 23:24 UTC (permalink / raw)
To: gcc-bugs
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.
^ permalink raw reply [flat|nested] 9+ messages in thread* [Bug target/54602] [SH] Register pop insn not put in rts delay slot
2012-09-16 15:26 [Bug target/54602] New: [SH] Register pop insn not put in rts delay slot olegendo at gcc dot gnu.org
` (4 preceding siblings ...)
2012-10-10 23:24 ` olegendo at gcc dot gnu.org
@ 2012-10-11 13:48 ` olegendo at gcc dot gnu.org
2012-10-12 23:23 ` olegendo at gcc dot gnu.org
2012-10-30 1:26 ` olegendo at gcc dot gnu.org
7 siblings, 0 replies; 9+ messages in thread
From: olegendo at gcc dot gnu.org @ 2012-10-11 13:48 UTC (permalink / raw)
To: gcc-bugs
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.
^ permalink raw reply [flat|nested] 9+ messages in thread* [Bug target/54602] [SH] Register pop insn not put in rts delay slot
2012-09-16 15:26 [Bug target/54602] New: [SH] Register pop insn not put in rts delay slot olegendo at gcc dot gnu.org
` (5 preceding siblings ...)
2012-10-11 13:48 ` olegendo at gcc dot gnu.org
@ 2012-10-12 23:23 ` olegendo at gcc dot gnu.org
2012-10-30 1:26 ` olegendo at gcc dot gnu.org
7 siblings, 0 replies; 9+ messages in thread
From: olegendo at gcc dot gnu.org @ 2012-10-12 23:23 UTC (permalink / raw)
To: gcc-bugs
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [Bug target/54602] [SH] Register pop insn not put in rts delay slot
2012-09-16 15:26 [Bug target/54602] New: [SH] Register pop insn not put in rts delay slot olegendo at gcc dot gnu.org
` (6 preceding siblings ...)
2012-10-12 23:23 ` olegendo at gcc dot gnu.org
@ 2012-10-30 1:26 ` olegendo at gcc dot gnu.org
7 siblings, 0 replies; 9+ messages in thread
From: olegendo at gcc dot gnu.org @ 2012-10-30 1:26 UTC (permalink / raw)
To: gcc-bugs
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.
^ permalink raw reply [flat|nested] 9+ messages in thread