Discussion:
[PATCH v2] GAS/MIPS: Add `-mfix-r5900' option for the R5900 short loop errata
Fredrik Noring
2018-10-27 09:37:32 UTC
Permalink
`-march=r5900' already enables the R5900 short loop workaround.
However, the R5900 ISA and most other MIPS ISAs are mutually
exclusive since R5900-specific instructions are generated as well.

The `-mfix-r5900' option can be used in combination with e.g.
`-mips2' or `-mips3' to generate generic MIPS binaries that also
work with the R5900 target.

This change has been tested with `make RUNTESTFLAGS=mips.exp
check-gas' for the targets `mipsr5900el-unknown-linux-gnu',
`mipsr5900el-elf' and `mips3-unknown-linux-gnu'.
---
Changes in v2:
- Discard `-mfix-r5900' option localisation
- Discard `.set reorder' from r5900-fix.s
- Swap `.ent' and label in r5900-fix.s
- Test `-mfix-r5900' border cases
- Honor and test `-mno-fix-r5900'
- Improve documentation wording
---
gas/config/tc-mips.c | 22 ++++++++++++++-
gas/doc/as.texi | 9 ++++++
gas/doc/c-mips.texi | 8 ++++++
gas/testsuite/gas/mips/mips.exp | 2 ++
gas/testsuite/gas/mips/r5900-fix.d | 30 ++++++++++++++++++++
gas/testsuite/gas/mips/r5900-fix.s | 40 +++++++++++++++++++++++++++
gas/testsuite/gas/mips/r5900-no-fix.d | 13 +++++++++
gas/testsuite/gas/mips/r5900-no-fix.s | 17 ++++++++++++
8 files changed, 140 insertions(+), 1 deletion(-)
create mode 100644 gas/testsuite/gas/mips/r5900-fix.d
create mode 100644 gas/testsuite/gas/mips/r5900-fix.s
create mode 100644 gas/testsuite/gas/mips/r5900-no-fix.d
create mode 100644 gas/testsuite/gas/mips/r5900-no-fix.s

diff --git a/gas/config/tc-mips.c b/gas/config/tc-mips.c
index 918525b4e9..3bfb8ebf32 100644
--- a/gas/config/tc-mips.c
+++ b/gas/config/tc-mips.c
@@ -939,6 +939,10 @@ static int mips_fix_rm7000;
/* ...likewise -mfix-cn63xxp1 */
static bfd_boolean mips_fix_cn63xxp1;

+/* ...likewise -mfix-r5900 */
+static bfd_boolean mips_fix_r5900;
+static bfd_boolean mips_fix_r5900_explicit;
+
/* We don't relax branches by default, since this causes us to expand
`la .l2 - .l1' if there's a branch between .l1 and .l2, because we
fail to compute the offset before expanding the macro to the most
@@ -1488,6 +1492,8 @@ enum options
OPTION_NO_FIX_VR4130,
OPTION_FIX_CN63XXP1,
OPTION_NO_FIX_CN63XXP1,
+ OPTION_FIX_R5900,
+ OPTION_NO_FIX_R5900,
OPTION_TRAP,
OPTION_BREAK,
OPTION_EB,
@@ -1636,6 +1642,8 @@ struct option md_longopts[] =
{"mno-fix-rm7000", no_argument, NULL, OPTION_NO_FIX_RM7000},
{"mfix-cn63xxp1", no_argument, NULL, OPTION_FIX_CN63XXP1},
{"mno-fix-cn63xxp1", no_argument, NULL, OPTION_NO_FIX_CN63XXP1},
+ {"mfix-r5900", no_argument, NULL, OPTION_FIX_R5900},
+ {"mno-fix-r5900", no_argument, NULL, OPTION_NO_FIX_R5900},

/* Miscellaneous options. */
{"trap", no_argument, NULL, OPTION_TRAP},
@@ -6997,7 +7005,8 @@ 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_fix_r5900_explicit ?
+ mips_fix_r5900 : mips_opts.arch == CPU_R5900)
/* Check if instruction has a parameter, ignore "j $31". */
&& (address_expr != NULL)
/* Parameter must be 16 bit. */
@@ -14763,6 +14772,16 @@ md_parse_option (int c, const char *arg)
mips_fix_cn63xxp1 = FALSE;
break;

+ case OPTION_FIX_R5900:
+ mips_fix_r5900 = TRUE;
+ mips_fix_r5900_explicit = TRUE;
+ break;
+
+ case OPTION_NO_FIX_R5900:
+ mips_fix_r5900 = FALSE;
+ mips_fix_r5900_explicit = TRUE;
+ break;
+
case OPTION_RELAX_BRANCH:
mips_relax_branch = 1;
break;
@@ -20125,6 +20144,7 @@ MIPS options:\n\
-mfix-vr4130 work around VR4130 mflo/mfhi errata\n\
-mfix-24k insert a nop after ERET and DERET instructions\n\
-mfix-cn63xxp1 work around CN63XXP1 PREF errata\n\
+-mfix-r5900 work around R5900 short loop errata\n\
-mgp32 use 32-bit GPRs, regardless of the chosen ISA\n\
-mfp32 use 32-bit FPRs, regardless of the chosen ISA\n\
-msym32 assume all symbols have 32-bit values\n\
diff --git a/gas/doc/as.texi b/gas/doc/as.texi
index acecd35225..1e00e7811b 100644
--- a/gas/doc/as.texi
+++ b/gas/doc/as.texi
@@ -453,6 +453,7 @@ gcc(1), ld(1), and the Info entries for @file{binutils} and @file{ld}.
[@b{-mfix-rm7000}] [@b{-mno-fix-rm7000}]
[@b{-mfix-vr4120}] [@b{-mno-fix-vr4120}]
[@b{-mfix-vr4130}] [@b{-mno-fix-vr4130}]
+ [@b{-mfix-r5900}] [@b{-mno-fix-r5900}]
[@b{-mdebug}] [@b{-no-mdebug}]
[@b{-mpdr}] [@b{-mno-pdr}]
@end ifset
@@ -1444,6 +1445,14 @@ 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
+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 fewer and always schedule a @code{nop} instruction there
+instead. The short loop bug under certain conditions causes loops to
+execute only once or twice, due to a hardware bug in the R5900 chip.
+
@item -mdebug
@itemx -no-mdebug
Cause stabs-style debugging output to go into an ECOFF-style .mdebug
diff --git a/gas/doc/c-mips.texi b/gas/doc/c-mips.texi
index 7751ce01d6..76dfb10794 100644
--- a/gas/doc/c-mips.texi
+++ b/gas/doc/c-mips.texi
@@ -327,6 +327,14 @@ 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
+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 fewer and always schedule a @code{nop} instruction there
+instead. The short loop bug under certain conditions causes loops to
+execute only once or twice, due to a hardware bug in the R5900 chip.
+
@item -m4010
@itemx -no-m4010
Generate code for the LSI R4010 chip. This tells the assembler to
diff --git a/gas/testsuite/gas/mips/mips.exp b/gas/testsuite/gas/mips/mips.exp
index 0da442c1d5..bc65917ffd 100644
--- a/gas/testsuite/gas/mips/mips.exp
+++ b/gas/testsuite/gas/mips/mips.exp
@@ -1560,6 +1560,8 @@ if { [istarget mips*-*-vxworks*] } {
run_dump_test_arches "break-error" [mips_arch_list_all]

run_dump_test "r5900"
+ run_dump_test "r5900-fix"
+ run_dump_test "r5900-no-fix"
run_dump_test "r5900-full"
run_list_test "r5900-nollsc" "-mabi=o64 -march=r5900"
run_dump_test "r5900-vu0"
diff --git a/gas/testsuite/gas/mips/r5900-fix.d b/gas/testsuite/gas/mips/r5900-fix.d
new file mode 100644
index 0000000000..faf482022a
--- /dev/null
+++ b/gas/testsuite/gas/mips/r5900-fix.d
@@ -0,0 +1,30 @@
+#objdump: -dr --prefix-addresses --show-raw-insn -M gpr-names=numeric -mmips:5900
+#name: MIPS R5900 workarounds (-mips3 -mfix-r5900)
+#as: -mips3 -mfix-r5900
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+[0-9a-f]+ <[^>]*> 2403012c li \$3,300
+[0-9a-f]+ <[^>]*> 2063ffff addi \$3,\$3,-1
+[0-9a-f]+ <[^>]*> 2084ffff addi \$4,\$4,-1
+[0-9a-f]+ <[^>]*> 1460fffd bnez \$3,[0-9a-f]+ <short_loop3>
+[0-9a-f]+ <[^>]*> 00000000 nop
+[0-9a-f]+ <[^>]*> 2403012c li \$3,300
+[0-9a-f]+ <[^>]*> 2063ffff addi \$3,\$3,-1
+[0-9a-f]+ <[^>]*> 2084ffff addi \$4,\$4,-1
+[0-9a-f]+ <[^>]*> 20a5ffff addi \$5,\$5,-1
+[0-9a-f]+ <[^>]*> 20c6ffff addi \$6,\$6,-1
+[0-9a-f]+ <[^>]*> 20e7ffff addi \$7,\$7,-1
+[0-9a-f]+ <[^>]*> 1460fffa bnez \$3,[0-9a-f]+ <short_loop6>
+[0-9a-f]+ <[^>]*> 00000000 nop
+[0-9a-f]+ <[^>]*> 2403012c li \$3,300
+[0-9a-f]+ <[^>]*> 2063ffff addi \$3,\$3,-1
+[0-9a-f]+ <[^>]*> 2084ffff addi \$4,\$4,-1
+[0-9a-f]+ <[^>]*> 20a5ffff addi \$5,\$5,-1
+[0-9a-f]+ <[^>]*> 20c6ffff addi \$6,\$6,-1
+[0-9a-f]+ <[^>]*> 20e7ffff addi \$7,\$7,-1
+[0-9a-f]+ <[^>]*> 1460fffa bnez \$3,[0-9a-f]+ <short_loop7>
+[0-9a-f]+ <[^>]*> 2108ffff addi \$8,\$8,-1
+[0-9a-f]+ <[^>]*> 24040003 li \$4,3
+ \.\.\.
diff --git a/gas/testsuite/gas/mips/r5900-fix.s b/gas/testsuite/gas/mips/r5900-fix.s
new file mode 100644
index 0000000000..97ab76cf1b
--- /dev/null
+++ b/gas/testsuite/gas/mips/r5900-fix.s
@@ -0,0 +1,40 @@
+ .text
+
+ .ent test_mfix_r5900
+test_mfix_r5900:
+ # Test the short loop fix with 3 loop instructions.
+ li $3, 300
+short_loop3:
+ addi $3, -1
+ addi $4, -1
+ # A NOP will be inserted in the branch delay slot.
+ bne $3, $0, short_loop3
+
+ # Test the short loop fix with 6 loop instructions.
+ li $3, 300
+short_loop6:
+ addi $3, -1
+ addi $4, -1
+ addi $5, -1
+ addi $6, -1
+ addi $7, -1
+ # A NOP will be inserted in the branch delay slot.
+ bne $3, $0, short_loop6
+
+ # Test the short loop fix with 7 loop instructions.
+ li $3, 300
+short_loop7:
+ addi $3, -1
+ addi $4, -1
+ addi $5, -1
+ addi $6, -1
+ addi $7, -1
+ addi $8, -1
+ # The short loop fix does not apply for loops with
+ # more than 6 instructions.
+ bne $3, $0, short_loop7
+
+ li $4, 3
+
+ .space 8
+ .end test_mfix_r5900
diff --git a/gas/testsuite/gas/mips/r5900-no-fix.d b/gas/testsuite/gas/mips/r5900-no-fix.d
new file mode 100644
index 0000000000..2ac60c9147
--- /dev/null
+++ b/gas/testsuite/gas/mips/r5900-no-fix.d
@@ -0,0 +1,13 @@
+#objdump: -dr --prefix-addresses --show-raw-insn -M gpr-names=numeric -mmips:5900
+#name: MIPS R5900 workarounds disabled (-mno-fix-r5900)
+#as: -march=r5900 -mtune=r5900 -mno-fix-r5900
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+[0-9a-f]+ <[^>]*> 2403012c li \$3,300
+[0-9a-f]+ <[^>]*> 2063ffff addi \$3,\$3,-1
+[0-9a-f]+ <[^>]*> 1460fffe bnez \$3,[0-9a-f]+ <short_loop_no_mfix_r5900>
+[0-9a-f]+ <[^>]*> 2084ffff addi \$4,\$4,-1
+[0-9a-f]+ <[^>]*> 24040003 li \$4,3
+ \.\.\.
diff --git a/gas/testsuite/gas/mips/r5900-no-fix.s b/gas/testsuite/gas/mips/r5900-no-fix.s
new file mode 100644
index 0000000000..723288976b
--- /dev/null
+++ b/gas/testsuite/gas/mips/r5900-no-fix.s
@@ -0,0 +1,17 @@
+ .text
+
+ .ent test_no_mfix_r5900
+test_no_mfix_r5900:
+ # Test that the short loop fix with 3 loop instructions
+ # is not applied with `-mno-fix-r5900'.
+ li $3, 300
+short_loop_no_mfix_r5900:
+ addi $3, -1
+ addi $4, -1
+ # A NOP will not be inserted in the branch delay slot.
+ bne $3, $0, short_loop_no_mfix_r5900
+
+ li $4, 3
+
+ .space 8
+ .end test_no_mfix_r5900
--
2.18.1
Maciej W. Rozycki
2018-11-27 16:58:48 UTC
Permalink
Hi Fredrik,

Thank you for your update. It goes in the right direction, but there is
still a nit I would like you to address.
Post by Fredrik Noring
`-march=r5900' already enables the R5900 short loop workaround.
However, the R5900 ISA and most other MIPS ISAs are mutually
exclusive since R5900-specific instructions are generated as well.
The `-mfix-r5900' option can be used in combination with e.g.
`-mips2' or `-mips3' to generate generic MIPS binaries that also
work with the R5900 target.
This change has been tested with `make RUNTESTFLAGS=mips.exp
check-gas' for the targets `mipsr5900el-unknown-linux-gnu',
`mipsr5900el-elf' and `mips3-unknown-linux-gnu'.
Please prepare a suitable ChangeLog entry. I have missed its absence in
the initial review somehow (sorry about that), but since we need another
iteration anyway, please include it alongside.
Post by Fredrik Noring
@@ -6997,7 +7005,8 @@ 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_fix_r5900_explicit ?
+ mips_fix_r5900 : mips_opts.arch == CPU_R5900)
The GNU coding standard requires operators to be included at the start of
the continuation line rather than at the end of one being wrapped, so this
would have to be rewritten as:

if ((mips_fix_r5900_explicit
? mips_fix_r5900 : mips_opts.arch == CPU_R5900)

or:

if ((mips_fix_r5900_explicit ? mips_fix_r5900
: mips_opts.arch == CPU_R5900)

or indeed:

if ((mips_fix_r5900_explicit ? mips_fix_r5900 : mips_opts.arch == CPU_R5900)

as this fits in the 79 column limit. However please set `mips_fix_r5900'
appropriately in `mips_after_parse_args' instead, i.e.:

if (!mips_fix_r5900_explicit)
mips_fix_r5900 = file_mips_opts.arch == CPU_R5900;

so that it's the only setting referred in determination as to whether to
enable the workaround. This will affect temporary `.set arch=' overrides,
but I think they are not supposed to override the global `-mfix-r5900'
setting, whether specified or inferred.

This is otherwise OK, so please resubmit with just this issue addressed.

Maciej
Fredrik Noring
2018-11-27 19:30:45 UTC
Permalink
Thank you for your review, Maciej!
Post by Maciej W. Rozycki
Please prepare a suitable ChangeLog entry. I have missed its absence in
the initial review somehow (sorry about that), but since we need another
iteration anyway, please include it alongside.
Done!
Post by Maciej W. Rozycki
However please set `mips_fix_r5900'
if (!mips_fix_r5900_explicit)
mips_fix_r5900 = file_mips_opts.arch == CPU_R5900;
so that it's the only setting referred in determination as to whether to
enable the workaround.
Done!
Post by Maciej W. Rozycki
This will affect temporary `.set arch=' overrides,
but I think they are not supposed to override the global `-mfix-r5900'
setting, whether specified or inferred.
Losing -mfix-r5900 inadvertently is fatal to the R5900 and will likely
cause corruption and any other imaginable error, so that must not happen.
This is also the main motivation to implement a warning for unfixable
cases involving for example the noreorder directive.
Post by Maciej W. Rozycki
This is otherwise OK, so please resubmit with just this issue addressed.
Great, will post v3 soon!

Fredrik

Loading...