Sorry for being a pain, but can I ping this? I wasn't sure if
anything was happening or not.
To quote the original message first:
Richard Sandiford <rdsandiford@googlemail.com> writes:
Catherine Moore <clm@codesourcery.com> writes:
This patch implements the errata for the 24K and 24KE. The errata
calls for a NOP to be emitted after an ERET/DERET instruction followed
by a branch. If the ERET/DERET is in a noreorder section, then a
warning will be issued instead. This functionality requires the
-mfix-24k option.
In addition, there was an error in the delay slot handling in
append_insn. The code failed to exclude an ERET or DERET instuction
from being moved into a delay slot. That failure is also corrected
with this patch.
Does this look okay to install?
Other hazards of this kind are handled by insns_between. Was there
a reason why you couldn't use it in this case?
I don't like the idea of warning about an ERET at the end of a
.set noreorder block. In general, if you have:
.set noreorder
...
FOO
...
.set reorder
...
BAR
and there is a hazard between FOO and BAR, it is perfectly OK to insert
nops at any point between the ".set reorder" and BAR. This is what we
do for other hazards, and I think we should be consistent. Likewise
if you have:
FOO
...
.set noreorder
...
BAR
...
.set reorder
it is perfectly OK to insert nops between FOO and the ".set noreorder".
(FWIW, using insns_between should give you this for free.)
Also, ".set neorder" has traditionally meant "trust me!". If we want
to warn about caess where we think the programmer is wrong, I think
we need a consistent story.
...but I probably wasn't as clear as I should have been.
-mfix-24k, as implemented by this patch, assumes that the assembler
should do nothing if the ERET or branch is in a ".set noreorder" block.
This isn't how the assembler handles other cases where nops are
needed between instruction A and instruction B. In these other cases,
the assembler inserts the required nops unless A and B are in the
_same_ ".set noreorder" block.
(If instruction A and instruction B are not in the same
".set noreorder" block, there must be at least one "reorderable
point" between A and B. The assembler will insert the nops at one
such point.)
I think it would be surprising for users if -mfix-24k hazards were
treated differently from all other hazards. Please consider using
insns_between instead!
To give a realistic example, suppose we have one function that
ends in an ERET and a following function that starts with a branch:
.globl foo
.set noreorder
.set nomacro
foo:
eret
.set macro
.set reorder
# REORDERABLE POINT
.globl bar
.set noreorder
.set nomacro
bar:
beq $4,$0,1f
sw $0,0($5)
sw $4,4($5)
1:
jr $31
nop
.set macro
.set reorder
Assemble this with -mfix-24k and you get:
foo.s:7: Warning: ERET and DERET must be followed by a NOP on the 24K.
foo.s:15: Warning: ERET and DERET must be followed by a NOP on the 24K.
and no nop is inserted between foo and bar:
00000000 <foo>:
0: 42000018 eret
00000004 <bar>:
4: 10800002 beqz a0,10 <bar+0xc>
8: aca00000 sw zero,0(a1)
c: aca40004 sw a0,4(a1)
10: 03e00008 jr ra
14: 00000000 nop
The correct behaviour here (IMO anyway) is not to issue a warning and
insert a nop at the end of foo. This is what would happen for other
cases where a nop is needed. Like I say, insns_between would give
you this behaviour for free.
The patch is also confused by ".ent" and ".end" markers, which is
why I left them out of the example above. If you have the following
code (where bar is intentionally different from before):
.globl foo
.ent foo
foo:
eret
.end foo
.globl bar
.ent bar
bar:
beq $4,$0,1f
sw $4,0($5)
1:
jr $31
.end bar
you get neither a warning nor a nop:
00000000 <foo>:
0: 42000018 eret
00000004 <bar>:
4: 10800002 beqz a0,10 <bar+0xc>
8: 00000000 nop
c: aca40000 sw a0,0(a1)
10: 03e00008 jr ra
14: 00000000 nop
As before, I believe the correct behaviour here is to silently insert
the nop at the end of foo. insns_between would again give you this
behaviour for free.
(I realise that, with insns_between, we currently don't have any code to
warn about cases where ".set noreorder" stopped us from inserting nops.
I'm certainly open to adding something though, and I think doing it that
way rather than the current way would avoid the double warning seen above.
However, as I said in the earlier message, I think we should be consistent
in the way we handle warnings as well as the way we handle nop insertion
itself.)
Richard