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

Hi Fredrik,

Thank you for your contribution. Your proposed change looks mostly good
to me, but I have one semantic change request, and then the usual bunch of
minor nits.

Post by Fredrik Noring
diff --git a/gas/config/tc-mips.c b/gas/config/tc-mips.c
index 918525b4e9..358c59f837 100644
--- a/gas/config/tc-mips.c
+++ b/gas/config/tc-mips.c
@@ -6997,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
+ 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.

Post by Fredrik Noring
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.
+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).

Post by Fredrik Noring
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.
certain CN63XXP1 errata.
+instructions or less.

Likewise.

Post by Fredrik Noring
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.

Post by Fredrik Noring
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
+
+ .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).

Post by Fredrik Noring
+ .set push
+ .set reorder

The `reorder' mode is the default, so you don't need them, and neither
`.set pop' below.

Post by Fredrik Noring
+ # Test the short loop fix with 3 loop instructions.
+ li $3, 300
+ 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.

Did you run regression-testing with your change? How did you verify it?

Maciej

Fredrik Noring

2018-10-26 19:37:13 UTC

Many thanks for your prompt review, Maciej!

Post by Fredrik Noring
- 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?

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
instead.
? I think it will be more precise (also we're inconsistent WRT
PDF output for visual quality).

Ah, yes, my description was a bit brief.

Likewise.

I will look into this one too.

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!

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.

Post by Fredrik Noring
+ .set push
+ .set reorder

The `reorder' mode is the default, so you don't need them, and neither
`.set pop' below.

Sure.

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. :)

Post by Maciej W. Rozycki
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

Maciej W. Rozycki

2018-10-26 20:12:03 UTC

Post by Fredrik Noring
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.

None of the other `-mfix-*' settings is implied by any of the `-march='
options though.

Post by Fredrik Noring
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?

Have a look at GCC, they have `target_flags_explicit' to tell defaulted
from explicit cases, and use that for their `-mfix-*' options. You can
use that as a template and adapt for our purposes here. You'll want to
have `-mfix-r5900' eventually implemented for GCC too, I suppose.

Post by Maciej W. Rozycki
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.

What target was that with though?

Post by Fredrik Noring
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)

But are these actual regressions or preexisting failures?

Post by Fredrik Noring
By the way, I'm still waiting for the paperwork from the FSF.

How come Nick has committed your previous change then? From that I
inferred the paperwork was sorted already (Nick as the head maintainer has
access to FSF copyright assignment records; I don't).

Maciej

Fredrik Noring

2018-10-27 06:24:38 UTC

Hi Maciej,

Post by Maciej W. Rozycki
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.

What target was that with though?

It was "mipsr5900el-unknown-linux-gnu".

But are these actual regressions or preexisting failures?

These are preexisting failures. Updating to HEAD this morning yields
two additional ones:

FAIL: MIPS MIPS64 MIPS-3D ASE instructions (-mips3d flag)
FAIL: MIPS MIPS64 MDMX ASE instructions (-mdmx flag)

As mentioned a while ago, there is also a preexisting build error with
the GDB part of Binutils:

CXX gdb.o
In file included from ../../gdb/defs.h:28:0,
from ../../gdb/gdb.c:19:
../../gdb/common/common-defs.h:71:0: error: "_FORTIFY_SOURCE" redefined [-Werror]
#define _FORTIFY_SOURCE 2

<built-in>: note: this is the location of the previous definition
cc1plus: all warnings being treated as errors
make[2]: *** [Makefile:1619: gdb.o] Error 1

I just filed a bug report for that here:

https://sourceware.org/bugzilla/show_bug.cgi?id=23835

I apply this patch as a workaround:

--- a/gdb/common/common-defs.h
+++ b/gdb/common/common-defs.h
@@ -67,7 +67,7 @@
optimization is required because _FORTIFY_SOURCE only works when
optimization is enabled. */

-#if defined __OPTIMIZE__ && __OPTIMIZE__ > 0
+#if defined __OPTIMIZE__ && __OPTIMIZE__ > 0 && 0
#define _FORTIFY_SOURCE 2
#endif

Fredrik

Maciej W. Rozycki

2018-10-27 07:25:16 UTC

Hi Fredrik,

Post by Maciej W. Rozycki
What target was that with though?

It was "mipsr5900el-unknown-linux-gnu".

Please mention that when making submissions.

I pushed your change through testing with my usual set of MIPS targets
and there were no issues, so things are looking good. Sometimes tweaks
are needed to otherwise reasonable test cases because of subtle
differences between various MIPS targets, for example such as these with
section alignment between Linux and bare metal ELF targets causing output
not to match the patterns expected.

You could include a bare metal ELF target in your testing, e.g.
`mipsr5900el-elf' would be an obvious choice in your case, to avoid
surprises.

Post by Maciej W. Rozycki
But are these actual regressions or preexisting failures?

These are preexisting failures.

There's no need to mention preexisting failures (unless you're fixing
them). Also I seem to remember now our discussion on these CP0 register
disassembly issues.

Post by Fredrik Noring
Updating to HEAD this morning yields
FAIL: MIPS MIPS64 MIPS-3D ASE instructions (-mips3d flag)
FAIL: MIPS MIPS64 MDMX ASE instructions (-mdmx flag)

Weird.

Post by Fredrik Noring
As mentioned a while ago, there is also a preexisting build error with

GDB is actually a separate project, although it shares the repository.
In the old days of the `src' CVS repo even more projects shared the tree,
and you'd normally check out just the CVS module you were interested in.
You can't do this with GIT, so instead you can omit non-binutils parts
from the build by adding `--disable-gdb --disable-libdecnumber
--disable-readline --disable-sim' to the `configure' invocation (you can
actually name arbitrary top-level subdirectories to exclude this way).

Likewise `--disable-binutils --disable-gas --disable-gold --disable-gprof
--disable-ld' will build the GDB parts only of the repository.

Post by Fredrik Noring
https://sourceware.org/bugzilla/show_bug.cgi?id=23835

Thanks!

Maciej

Fredrik Noring

2018-10-27 09:22:31 UTC

Hi Maciej,