[PATCH] GAS/MIPS: Add `-mfix-r5900' option for the R5900 short loop errata

Fredrik Noring noring@nocrew.org
Fri Oct 26 19:37:00 GMT 2018


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



More information about the Binutils mailing list