[patch] [MIPS] Implement Errata for 24K and 24KE

[Catherine Moore]

Hi Richard,
I saw you original note.  I'm on vacation right now.  Is it okay if I address your issues when I return?
Thanks,
Catherine

Richard Sandiford wrote:
> 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