This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] GAS/MIPS: Add `-mfix-r5900' option for the R5900 short loop errata
Many thanks for your prompt review, Maciej!
> > - if (mips_opts.arch == CPU_R5900
> > + if ((mips_opts.arch == CPU_R5900 || mips_fix_r5900)
>
> Please make `-mfix-r5900' independent from `-march=r5900', so that
> `-march=r5900 -mno-fix-r5900' (or `-mno-fix-r5900 -march=r5900') does not
> enable the workaround. I'm happy with the default for `-march=r5900' to
> be `-mfix-r5900' of course, reflecting the current situation, but please
> document it in the manual too.
>
> The reason is we always want to be able to flip each option off, as
> documented anyway.
I have been thinking about something along the way of this:
--- a/gas/config/tc-mips.c
+++ b/gas/config/tc-mips.c
@@ -7004,7 +7004,7 @@ can_swap_branch_p (struct mips_cl_insn *ip, expressionS *address_expr,
- a branch delay slot of the loop is not NOP (EE 2.9 or later).
We need to do this because of a hardware bug in the R5900 chip. */
- if ((mips_opts.arch == CPU_R5900 || mips_fix_r5900)
+ if (mips_fix_r5900
/* Check if instruction has a parameter, ignore "j $31". */
&& (address_expr != NULL)
/* Parameter must be 16 bit. */
@@ -15045,6 +15045,11 @@ mips_after_parse_args (void)
as_bad (_("-march=%s is not compatible with the selected ABI"),
arch_info->name);
+ if (arch_info->cpu == CPU_R5900)
+ {
+ mips_fix_r5900 = TRUE;
+ }
+
file_mips_opts.arch = arch_info->cpu;
file_mips_opts.isa = arch_info->isa;
Then -march=r5900 implies -mfix-r5900, which means a target such as
mipsr5900-unknown-linux-gnu automatically has -mfix-r5900 even when
generating code with -mips3 (without explicitly given -mfix-r5900).
I thought that would provide the best guarantees of generating R5900
compatible code, under the broadest circumstances. However, none of
the other -mfix-* options seem to operate like that, so I scratched
that idea. Besides, it also failed to handle -mno-fix-r5900.
Given that the variable mips_fix_r5900 is TRUE or FALSE, should I use
another variable or mechanism to check whether the option is actually
given as opposed to being implied? How to tell the difference?
> > diff --git a/gas/doc/as.texi b/gas/doc/as.texi
> > index acecd35225..1270e6efa1 100644
> > --- a/gas/doc/as.texi
> > +++ b/gas/doc/as.texi
> > @@ -1444,6 +1445,11 @@ of an mfhi or mflo instruction occurs in the following two instructions.
> > Cause nops to be inserted if a dmult or dmultu instruction is
> > followed by a load instruction.
> >
> > +@item -mfix-r5900
> > +@itemx -mno-fix-r5900
> > +Insert a NOP in the branch delay slot for short loops with six
> > +instructions or less.
>
> How about:
>
> Do not attempt to schedule the preceding instruction into the delay slot
> of a branch instruction placed at the end of a short loop of six
> instructions or less and always schedule a @code{nop} instruction there
> instead.
>
> ? I think it will be more precise (also we're inconsistent WRT
> instruction markup, but I think @code fits best here; if in doubt check
> PDF output for visual quality).
Ah, yes, my description was a bit brief.
> > diff --git a/gas/doc/c-mips.texi b/gas/doc/c-mips.texi
> > index 7751ce01d6..3ee407924c 100644
> > --- a/gas/doc/c-mips.texi
> > +++ b/gas/doc/c-mips.texi
> > @@ -327,6 +327,11 @@ Insert nops to work around the 24K @samp{eret}/@samp{deret} errata.
> > Replace @code{pref} hints 0 - 4 and 6 - 24 with hint 28 to work around
> > certain CN63XXP1 errata.
> >
> > +@item -mfix-r5900
> > +@itemx -mno-fix-r5900
> > +Insert a @samp{nop} in the branch delay slot for short loops with six
> > +instructions or less.
>
> Likewise.
I will look into this one too.
> > diff --git a/gas/po/sv.po b/gas/po/sv.po
> > index 575bf0621d..cee660b315 100644
> > --- a/gas/po/sv.po
> > +++ b/gas/po/sv.po
> > @@ -12546,6 +12546,7 @@ msgid ""
> > "-mfix-vr4130\t\twork around VR4130 mflo/mfhi errata\n"
> > "-mfix-24k\t\tinsert a nop after ERET and DERET instructions\n"
> > "-mfix-cn63xxp1\t\twork around CN63XXP1 PREF errata\n"
> > +"-mfix-r5900\t\twork around R5900 short loop errata\n"
> > "-mgp32\t\t\tuse 32-bit GPRs, regardless of the chosen ISA\n"
> > "-mfp32\t\t\tuse 32-bit FPRs, regardless of the chosen ISA\n"
> > "-msym32\t\t\tassume all symbols have 32-bit values\n"
> > @@ -12560,6 +12561,7 @@ msgstr ""
> > "-mfix-vr4130\t\tlösning för VR4130 mflo/mfhi-errata\n"
> > "-mfix-24k\t\tinfoga en nop efter ERET- och DERET-instruktioner\n"
> > "-mfix-cn63xxp1\t\tlösning för CN63XXP1 PREF-errata\n"
> > +"-mfix-r5900\t\tlösning för R5900 kort loop-errata\n"
> > "-mgp32\t\t\tanvänd 32-bitars GPRs, oberoende av vald ISA\n"
> > "-mfp32\t\t\tanvänd 32-bitars FPRs, oberoende av vald ISA\n"
> > "-msym32\t\t\tantag att alla symbole har 32-bitars värden\n"
>
> This stuff is normally done by the translation team shortly before a
> release, so I believe it will best be dropped at this stage.
OK!
> > diff --git a/gas/testsuite/gas/mips/r5900-fix.s b/gas/testsuite/gas/mips/r5900-fix.s
> > new file mode 100644
> > index 0000000000..167eb2c69f
> > --- /dev/null
> > +++ b/gas/testsuite/gas/mips/r5900-fix.s
> > @@ -0,0 +1,19 @@
> > + .text
> > +
> > +test_mfix_r5900:
> > + .ent test_mfix_r5900
>
> Please swap these two lines as `.ent' is supposed to precede the label
> referred (we may not actually enforce that, but let's keep code right).
Sure.
> > + .set push
> > + .set reorder
>
> The `reorder' mode is the default, so you don't need them, and neither
> `.set pop' below.
Sure.
> > + # Test the short loop fix with 3 loop instructions.
> > + li $3, 300
> > +short_loop_mfix_r5900:
> > + addi $3, -1
> > + addi $4, -1
> > + # A NOP will be inserted in the branch delay slot.
> > + bne $3, $0, short_loop_mfix_r5900
> > +
> > + li $4, 3
> > + .set pop
> > +
> > + .space 8
> > + .end test_mfix_r5900
>
> Can you make this test verify boundary cases? It would be more useful
> than picking a random size of a loop from the middle of the affected range
> (which of course you can keep regardless). We want to make sure we get
> the range right, now and forever.
Yes, since I added those to r5900.d a couple of weeks ago, I can easily
duplicate the whole short loop fix test including the boundary cases. :)
> Did you run regression-testing with your change? How did you verify it?
Yes, I checked it with "make RUNTESTFLAGS=mips.exp check-gas", including
provisionally discarding -mfix-r5900 from the file r5900-fix.d to verify
that a test failure was triggered.
As usual, six unrelated tests fail in the R5900 target test suite:
FAIL: MIPS CP0 register disassembly (numeric)
FAIL: MIPS CP0 register disassembly (mips32)
FAIL: MIPS CP0 register disassembly (mips32r2)
FAIL: MIPS CP0 register disassembly (mips64)
FAIL: MIPS CP0 register disassembly (mips64r2)
FAIL: MIPS CP0 register disassembly (sb1)
By the way, I'm still waiting for the paperwork from the FSF.
Fredrik