[patch] [MIPS] Implement Errata for 24K and 24KE
Richard Sandiford
rdsandiford@googlemail.com
Wed May 20 20:08:00 GMT 2009
Catherine Moore <clm@codesourcery.com> writes:
> I've attached a new patch. It adds the testcases that you asked for
> along with the other cleanups. I had trouble with one section -- this
> first example:
>
> .set noreorder
> 1: eret
> .set reorder
> b 1b
>
> insns_between is called only when the the history insn is not in a
> noreorder block.
Hmm, yeah. So the current code doesn't behave as I said after all. ;(
Sorry about that. Serves me right for going on memory.
I still think that what I said is what ought to happen. I can't see
any reason for the current approach to non-24k hazards, where we insert
nops between A and B iff _A_ is not in a noreorder block. Why should
A and B be assymetric in that way?
The only test that relies on the current behaviour is the one for
-mfix-vr4130, but I think I was slavishly following existing practice
when I added that. So the test requires:
.set noreorder
mfhi ...
.end noreorder
mult ...
to insert no nops. But a programmer could sensibly write:
.set noreorder
beq $2,$0,foo
mfhi ...
.end noreorder
mult ...
if they knew that foo didn't use HI or LO. I don't think it should
be taken as a statement that "nothing after this set noreorder block
conflicts with it".
I don't have access to the IRIX assembler any more, but the following
comment is interesting:
/* If the previous previous insn was in a .set
noreorder, we can't swap. Actually, the MIPS
assembler will swap in this situation. However, gcc
configured -with-gnu-as will generate code like
.set noreorder
lw $4,XXX
.set reorder
INSN
bne $4,$0,foo
in which we can not swap the bne and INSN. If gcc is
not configured -with-gnu-as, it does not output the
.set pseudo-ops. */
|| history[1].noreorder_p
The traditional MIPS assembler behaviour described here seems to
reinforce the idea that ".set noreorder" blocks are simply atomic
blocks of insns, and that their atomicness shouldn't affect their
interaction with insns outside the block.
So I'm inclined to apply the attached patch, which implements
the semantics I was describing in my earlier messages. I'll leave
it till the weekend for objections.
(FWIW:
> *************** nops_for_insn (const struct mips_cl_insn
> *** 2714,2720 ****
>
> nops = 0;
> for (i = 0; i < MAX_DELAY_NOPS; i++)
> ! if (!history[i].noreorder_p)
> {
> tmp_nops = insns_between (history + i, insn) - i;
> if (tmp_nops > nops)
> --- 2658,2665 ----
>
> nops = 0;
> for (i = 0; i < MAX_DELAY_NOPS; i++)
> ! if (!history[i].noreorder_p
> ! || (mips_fix_24k && insn && !insn->noreorder_p))
> {
> tmp_nops = insns_between (history + i, insn) - i;
> if (tmp_nops > nops)
wouldn't really have been OK, since it would make -mfix-24k affect
the behaviour of all nops, just those related to -mfix-24k itself.)
So assuming we can use the patch below, and drop the hunk above
from your patch, the rest looks good. However, the logic is
overly complex:
> + /* If we're working around 24K errata, one instruction is required
> + if an ERET or DERET is followed by a branch instruction. */
> + if (mips_fix_24k)
> + {
> + if (insn1 != NULL
> + && (insn1->insn_opcode == INSN_ERET
> + || insn1->insn_opcode == INSN_DERET)
> + && insn2 == NULL)
> + return 1;
> +
> + if ((insn1 != NULL)
> + && (insn2 != NULL)
> + && (insn1->insn_opcode == INSN_ERET
> + || insn1->insn_opcode == INSN_DERET)
> + && ((insn2->insn_opcode == INSN_ERET)
> + || (insn2->insn_opcode == INSN_DERET)
> + || ((insn2->insn_mo->pinfo
> + & (INSN_UNCOND_BRANCH_DELAY
> + | INSN_COND_BRANCH_DELAY
> + | INSN_COND_BRANCH_LIKELY)))))
> + return 1;
> + }
Since insn1 can't be null, this reduces to:
if (mips_fix_24k)
{
if (insn1->insn_opcode == INSN_ERET
|| insn1->insn_opcode == INSN_DERET)
{
if (insn2 == NULL
|| insn2->insn_opcode == INSN_ERET
|| insn2->insn_opcode == INSN_DERET
|| (insn2->insn_mo->pinfo
& (INSN_UNCOND_BRANCH_DELAY
| INSN_COND_BRANCH_DELAY
| INSN_COND_BRANCH_LIKELY)) != 0)
return 1;
}
}
The patch is OK to apply with those changes, assuming the patch
below goes in first.
Thanks for your patience and for being prepared to rework the patch!
Richard
gas/
* config/tc-mips.c (nops_for_vr4130): Don't check noreorder_p.
(nops_for_insn): Likewise.
gas/testsuite/
* gas/mips/vr4130.s, gas/mips/vr4130.d: Expect part A to have nops.
Index: gas/config/tc-mips.c
===================================================================
--- gas/config/tc-mips.c 2009-05-20 20:49:14.000000000 +0100
+++ gas/config/tc-mips.c 2009-05-20 20:54:25.000000000 +0100
@@ -2679,7 +2679,7 @@ nops_for_vr4130 (const struct mips_cl_in
/* Search for the first MFLO or MFHI. */
for (i = 0; i < MAX_VR4130_NOPS; i++)
- if (!history[i].noreorder_p && MF_HILO_INSN (history[i].insn_mo->pinfo))
+ if (MF_HILO_INSN (history[i].insn_mo->pinfo))
{
/* Extract the destination register. */
if (mips_opts.mips16)
@@ -2714,12 +2714,11 @@ nops_for_insn (const struct mips_cl_insn
nops = 0;
for (i = 0; i < MAX_DELAY_NOPS; i++)
- if (!history[i].noreorder_p)
- {
- tmp_nops = insns_between (history + i, insn) - i;
- if (tmp_nops > nops)
- nops = tmp_nops;
- }
+ {
+ tmp_nops = insns_between (history + i, insn) - i;
+ if (tmp_nops > nops)
+ nops = tmp_nops;
+ }
if (mips_fix_vr4130)
{
Index: gas/testsuite/gas/mips/vr4130.s
===================================================================
--- gas/testsuite/gas/mips/vr4130.s 2009-05-20 20:49:14.000000000 +0100
+++ gas/testsuite/gas/mips/vr4130.s 2009-05-20 20:54:25.000000000 +0100
@@ -16,8 +16,7 @@
# PART A
#
- # Check that mfhis and mflos in .set noreorder blocks are not
- # considered.
+ # Check that mfhis and mflos in .set noreorder blocks are considered.
.set noreorder
mfhi $2
Index: gas/testsuite/gas/mips/vr4130.d
===================================================================
--- gas/testsuite/gas/mips/vr4130.d 2009-05-20 20:49:14.000000000 +0100
+++ gas/testsuite/gas/mips/vr4130.d 2009-05-20 20:54:25.000000000 +0100
@@ -11,9 +11,17 @@ Disassembly.*
# PART A
#
.* mfhi .*
+.* nop
+.* nop
+.* nop
+.* nop
.* mult .*
#
.* mflo .*
+.* nop
+.* nop
+.* nop
+.* nop
.* mult .*
#
# PART B
@@ -426,9 +434,17 @@ Disassembly.*
# PART A
#
.* mfhi .*
+.* nop
+.* nop
+.* nop
+.* nop
.* mult .*
#
.* mflo .*
+.* nop
+.* nop
+.* nop
+.* nop
.* mult .*
#
# PART B
More information about the Binutils
mailing list