Re: RFC: MIPS: Workaround the "daddi" and "daddiu" errata

["Maciej W. Rozycki" <macro at ds2 dot pg dot gda dot pl>]

On Tue, 1 Jun 2004, Richard Sandiford wrote:

> >  How can any ELF feature tell you there are no bad instructions in code in 
> > a binary?  Any flags or special sections may at most show an effort was 
> > done to deal with the problem.  This is useful, but not completely safe.  
> 
> That seems like a spurious argument to me.  If an ELF executable
> identifies itself as VR5500 code, for example, how do you know you
> know it's _really_ VR500 code, and there aren't any MIPS64 instructions
> (or whatever) lurking in there as well?  You don't.  You just have to
> trust whatever tool produced the executable (and fix that tool if does
> the wrong thing).

 Well, you may examine the binary for this, too -- that's no problem.  
But this case is easier -- an invalid instruction will trigger a reserved
instruction exception.  It already happened for me in the past for a few
times, with an R3k processor when running code for which someone assumed
the MIPS II ISA (with inline assembly).

 The ability to do such verification is of course non-essential for the
workaround, but it would be useful for a quick check of a suspect binary 
that behaves strangely on a problematic processor and correctly elsewhere.

> >  Well, something like:
> >
> > ((void (*)(void))0xffffffffbfc00000)();
> >
> > used to work for me to call the reset vector from Linux. ;-)  There's no
> > need to resort to linker scripts to create arbitrary pointers -- the
> > expression above could be changed to define e.g. an array.
> 
> Maciej, it feels like you're being deliberately awkwrd/obtuse here.

 That's not my intent -- I'm sorry if it looks like that.

> Of course ((void (*)(void))0xffffffffbfc00000) is a valid object.
> It's also a valid pointer.  You still haven't shown any examples
> of valid objects references based on an _invalid_ pointer.

 My point is it is technically possible and I feel uneasy of a compiler
creating code that's good or bad, depending on some numerical values of
data items.

> >  The patch is tested with a snapshot of Linux 2.4.24 and appears to work
> > fine -- I'm still debugging a problem with the fcntl syscall, but I think
> > it's unrelated, being more a generic gcc 3.5.x compatibility problem, like
> > a few I've already resolved.
> >
> >  What do you think?
> 
> Well, I spent a significant amount of time reviewing your previous
> iterations on this patch and explaining what I thought should happen.
> You don't seem to have made any real attempt to address my concerns.

 I did whatever I could without losing functionality.  In particular I
removed code duplication for all affected patterns operating on memory
(which I believe was the most significant maintainability concern) and
addressed the adddi3 and ffsdi patterns.  At this point there was no sense 
to consider disabling the workaround for the no-explictit-relocs case as 
the effort required got reduced to a single condition in the 
EXTRA_CONSTRAINT macro.

> So as far as I'm concerned, the patch is still unacceptable, for the
> same reasons I gave earlier.  Feel free to try to convince someone
> else to approve it instead though.

 Unfortunately, after the mentioned changes, I fail to see what'd be going
to be, as you've stated, hard to maintain in the long-term.  Perhaps the
stack manipulation macros would be a bit troublesome as they're sequences
of a few lines, that would become duplicated.  There may be a way to avoid 
that duplication, but I seriously doubt if the resulting code would be 
clearer.  And some of the complication is a result of the gcc's 
requirement to be able to jump to an arbitrary address without clobbering 
any registers.  Which is of course impossible for MIPS.  I don't know if 
the requirement will be relaxed in the future.

 To summarize the discussion, which there's no point to keep continued
IMO, I propose to record our difference in opinions in the minutes.  I'll
keep my set of changes maintained off the tree -- I have done it for
almost two years now, so I see no problem to continue that effort.  The
results have been available at my FTP site and they'll continue to be.

 As there's a part of changes we both agree on, I propose to have it 
included anyway, for the sake of those who would be satisfied with a 
partial workaround, accompanied, of course, with appropriate notes on the 
limited nature of the code.  Here's what I think constitutes the best of 
the lesser evil.  It's been roughly tested by building a minimal compiler 
(I still have no mips64 glibc, and I don't expect to get one soon enough) 
along the lines of:

$ configure --enable-languages=c --disable-libmudflap --disable-shared 
--enable-static --host=i386-linux --target=mips64el-linux
$ make all

which completed successfully and then using the resulting program to build
a Linux kernel in its 64-bit configuration, with the workaround enabled.  
The resulting code has correct sequences for the chosen additions making
use of a "li" to $at and then a "daddu".  Additionally the kernel runs
more or less OK -- the fcntl() syscall problem persists, but as I've
already written, I believe it to be a generic Linux vs gcc 3.5
compatibility problem.

 I'll send the gas part to the binutils list shortly, as soon as I update
it accordingly and do some testing.  My approach is going to be the same
(there's no other choice with this minimal support from the gcc side,
anyway) -- the update is going to be a complementary change, with the
complete set maintained outside the tree.

2004-06-03  Richard Sandiford  <rsandifo@redhat.com>
	    Maciej W. Rozycki  <macro@ds2.pg.gda.pl>

	* config/mips/mips.md: Add a partial workaround for the R4000 and 
	R4400 "daddiu" erratum.  Add documentation.
	(adddi3_internal): Take the workaround into account.
	(adddi3_internal_r4000): New pattern to actually implement it.
	* doc/invoke.texi: Document the workaround.

[It's nice to have addresses of the same length. ;-)  Unfortunately,
this'll change as I need to move soon.]

 Please accept my apologies for taking your precious time -- I hope you 
don't consider it completely wasted.

  Maciej

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

gcc-3.5.0-20040524-mips-nodaddi-crippled.patch
diff -up --recursive --new-file gcc-3.5.0-20040524.macro/gcc/config/mips/mips.md gcc-3.5.0-20040524/gcc/config/mips/mips.md
--- gcc-3.5.0-20040524.macro/gcc/config/mips/mips.md	2004-05-18 01:31:11.000000000 +0000
+++ gcc-3.5.0-20040524/gcc/config/mips/mips.md	2004-06-03 00:52:37.000000000 +0000
@@ -889,6 +889,41 @@
     }
 })
 
+;; The original R4000 and the initial revision of the R4400 have a cpu
+;; bug.  Under an overflow condition double-word immediate addition
+;; may give an incorrect result.  We handle the problem in selected
+;; cases by using a sequence of a single-word immediate addition to
+;; load a constant to a temporary register and then a double-word
+;; register addition.  Our workaround is not completely fail-proof,
+;; but it deals with a common subset of failure condidtions.
+;;
+;; From "MIPS R4000PC/SC Errata, Processor Revision 2.2 and 3.0"
+;; (also valid for MIPS R4000MC processors):
+;;
+;; "23. R4000PC, R4000SC: The 64-bit instruction, daddi, fails to take
+;;	an overflow exception.
+;;	Workaround: There is no workaround for this problem."
+;;
+;; and:
+;;
+;; "41. R4000PC, R4000SC: Under the following condition, the DADDIU
+;;	instruction can produce an incorrect result.  If this
+;;	instruction generates a result value that would cause an
+;;	overflow condition to occur (even though this instruction does
+;;	not take an overflow exception) then the result value will be
+;;	correct in bits 0-31 but bit 31 will be replicated through
+;;	bits 32-63 (so it looks like a 32bit signextended value).  The
+;;	overflow condition is defined when the carries out of bits 62
+;;	and 63 differ (two's compliment overflow).
+;;	Workaround: There is no workaround for this problem."
+;;
+;; Erratum #41 is also present in "MIPS R4400PC/SC Errata, Processor
+;; Revision 1.0" (also valid for MIPS R4400MC processors) as erratum
+;; #7.
+;;
+;; These processors have PRId values of 0x00004220 and 0x00004300 for
+;; the R4000 and 0x00004400 for the R4400.
+
 (define_expand "adddi3"
   [(set (match_operand:DI 0 "register_operand")
 	(plus:DI (match_operand:DI 1 "register_operand")
@@ -917,17 +952,37 @@
     }
 })
 
-(define_insn "adddi3_internal"
+(define_insn "*adddi3_internal"
   [(set (match_operand:DI 0 "register_operand" "=d,d")
 	(plus:DI (match_operand:DI 1 "reg_or_0_operand" "dJ,dJ")
 		 (match_operand:DI 2 "arith_operand" "d,Q")))]
-  "TARGET_64BIT && !TARGET_MIPS16"
+  "TARGET_64BIT && !TARGET_MIPS16 && !(TARGET_FIX_R4000 || TARGET_FIX_R4400)"
   "@
     daddu\t%0,%z1,%2
     daddiu\t%0,%z1,%2"
   [(set_attr "type"	"arith")
    (set_attr "mode"	"DI")])
 
+(define_insn "*adddi3_internal_r4000"
+  [(set (match_operand:DI 0 "register_operand" "=d,d")
+	(plus:DI (match_operand:DI 1 "reg_or_0_operand" "dJ,dJ")
+		 (match_operand:DI 2 "arith_operand" "d,Q")))]
+  "TARGET_64BIT && !TARGET_MIPS16 && (TARGET_FIX_R4000 || TARGET_FIX_R4400)"
+{
+  if (which_alternative == 0)
+    return "daddu\t%0,%z1,%2";
+  else if ((GET_CODE (operands[1]) == REG
+	    && REGNO (operands[1]) == STACK_POINTER_REGNUM)
+	   || (GET_CODE (operands[1]) == CONST_INT
+	       && (INTVAL (operands[1]) == 0)))
+    return "daddiu\t%0,%z1,%2";
+  else
+    return "%[addiu\\t%@,%.,%2\;daddu\t%0,%1,%@%]";
+}
+  [(set_attr "type"	"arith")
+   (set_attr "mode"	"DI")
+   (set_attr "length"	"4,8")])
+
 ;; For the mips16, we need to recognize stack pointer additions
 ;; explicitly, since we don't have a constraint for $sp.  These insns
 ;; will be generated by the save_restore_insns functions.
diff -up --recursive --new-file gcc-3.5.0-20040524.macro/gcc/doc/invoke.texi gcc-3.5.0-20040524/gcc/doc/invoke.texi
--- gcc-3.5.0-20040524.macro/gcc/doc/invoke.texi	2004-05-23 01:30:17.000000000 +0000
+++ gcc-3.5.0-20040524/gcc/doc/invoke.texi	2004-06-03 00:50:36.000000000 +0000
@@ -8409,6 +8409,9 @@ while an integer multiplication is in pr
 @item
 An integer division may give an incorrect result if started in a delay slot
 of a taken branch or a jump.
+@item
+The @code{daddiu} instruction can produce an incorrect result (this is only
+dealt with for a common subset of failure conditions).
 @end itemize
 
 @item -mfix-r4400
@@ -8420,6 +8423,9 @@ Work around certain R4400 CPU errata:
 @item
 A double-word or a variable shift may give an incorrect result if executed
 immediately after starting an integer division.
+@item
+The @code{daddiu} instruction can produce an incorrect result (this is only
+dealt with for a common subset of failure conditions).
 @end itemize
 
 @item -mfix-vr4120