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

Message ID 17c89c52e61cf1f6c4f365c3c25e89d28f096062.1540632453.git.noring@nocrew.org
State Superseded
Headers show
Series
  • [v2] GAS/MIPS: Add `-mfix-r5900' option for the R5900 short loop errata
Related show

Commit Message

Fredrik Noring Oct. 27, 2018, 9:37 a.m.
`-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

-- 
2.18.1

Comments

Maciej W. Rozycki Nov. 27, 2018, 4:58 p.m. | #1
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.

> `-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.

> @@ -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 Nov. 27, 2018, 7:30 p.m. | #2
Thank you for your review, Maciej!

>  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!

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


Done!

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

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


Great, will post v3 soon!

Fredrik

Patch

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