[PATCH] MIPS: Fix GCC `noreorder' for undefined R5900 short loops

[Fredrik Noring <noring@nocrew.org>]

Hi Paul, Matthew,

Paul -- as I'm preparing the R5900 kernel patches there was a USB DMA series
and breakage that needed attention. The fixes ending with ff2437befd8f ("usb:
host: Fix excessive alignment restriction for local memory allocations") are
now merged with Linus' kernel, and recommended for the R5900 series. The
initial R5900 patch submission is getting closer. :)

The present problem is related to GCC and the R5900 short loop bug[1]. It
turns out GCC emits assembly code like

loop:	addiu	$5,$5,1
	addiu	$4,$4,1
	lb	$2,-1($5)
	.set	noreorder
	.set	nomacro
	bne	$2,$3,loop
	sb	$2,-1($4)
	.set	macro
	.set	reorder

that is undefined for the R5900 (this short loop has five instructions),
for simple C code such as

	while ((*s++ = *p++) != '\n')
		;

in the kernel. The noreorder directive prohibits GAS from corrections, and
GAS really ought to give an error for it, I think. In the meantime, I have a
tool that does machine code analysis of ELF objects to catch undefined R5900
short loops, including those made with assembler macros in the kernel.

[ In theory, GAS could actually insert NOPs prior to the noreorder directive,
to make the loop longer that six instructions, but GAS does not have that
kind of capability. Another option that GCC prevents is to place a NOP in
the delay slot. ]

A reasonable fix for GCC is perhaps to update gcc/config/mips/mips.md to not
make explicit use of the branch delay slot, as suggested by the patch below?
Then GCC will emit

loop:	addiu	$5,$5,1
	addiu	$4,$4,1
	lb	$2,-1($5)
	sb	$2,-1($4)
	bne	$2,$3,loop

that GAS will adjust in the ELF object to

   4:	24a50001 	addiu	a1,a1,1
   8:	24840001 	addiu	a0,a0,1
   c:	80a2ffff 	lb	v0,-1(a1)
  10:	a082ffff 	sb	v0,-1(a0)
  14:	1443fffb 	bne	v0,v1,4
  18:	00000000 	nop

where a NOP is placed in the delay slot to avoid the bug. For longer loops,
this relies on GAS making appropriate use of the delay slot. I'm not sure if
GAS is good at that, though?

Fredrik

References:

[1] https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gas/config/tc-mips.c;h=b7b4b6989a12d3091a02de7155fbea3adbf1c9d7;hb=HEAD#l7133

    On the R5900 short loops need to be fixed by inserting a NOP in the
    branch delay slot.
    
    The short loop bug under certain conditions causes loops to execute
    only once or twice.  We must ensure that the assembler never
    generates loops that satisfy all of the following conditions:
    
    - a loop consists of less than or equal to six instructions
      (including the branch delay slot);
    - a loop contains only one conditional branch instruction at the end
      of the loop;
    - a loop does not contain any other branch or jump instructions;
    - 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.

diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md
index e17b1d522f0..acd31a8960c 100644
--- a/gcc/config/mips/mips.md
+++ b/gcc/config/mips/mips.md
@@ -1091,6 +1091,7 @@
 
 (define_delay (and (eq_attr "type" "branch")
 		   (not (match_test "TARGET_MIPS16"))
+		   (not (match_test "TARGET_FIX_R5900"))
 		   (eq_attr "branch_likely" "yes"))
   [(eq_attr "can_delay" "yes")
    (nil)
@@ -1100,6 +1101,7 @@
 ;; not annul on false.
 (define_delay (and (eq_attr "type" "branch")
 		   (not (match_test "TARGET_MIPS16"))
+		   (not (match_test "TARGET_FIX_R5900"))
 		   (ior (match_test "TARGET_CB_NEVER")
 			(and (eq_attr "compact_form" "maybe")
 			     (not (match_test "TARGET_CB_ALWAYS")))