Re: [rfa/mips] Second go at vr5500 hilo hazard fix

[cgd@broadcom.com]

[-- Attachment #1: Type: text/plain, Size: 2265 bytes --]

At Thu, 18 Mar 2004 15:06:42 +0000 (UTC), "Richard Sandiford" wrote:
> Various suggestions were made about how this could be handled, but I
> think Andrew's position remained the same: we shouldn't try to treat the
> vr5500 ISA as "MIPS IV plus a bit and minus a bit" (my words, not his).
> He reckoned every vr5500 instruction should be marked as such.

My reading is, unfortunately, is that it is a *correct* implementation
of the MIPS ISA.

At least according to the "Historical information" in the MIPS64 AFP
Volume II (Basic Instruction Set) -- I'm looking at revisions around
1.0 here -- the 3-cycle hi/lo hazards should only have been a problem
for MIPS I-III.

I.e., MIPS IV processors which *have* these hi/lo hazards are the
things broken... but as you note at least according to the 5400 docs,
it is MIPS IV and does have the hazard.


(I thought Andrew's biggest objection was stuffing bfd_* values into
mips.igen, but I may be wrong on that.  It's been a while.)


> The patch below does this.  Tested on mips64vrel-elf.  OK to install?

Doing this doesn't seem satisfactory to me.  It would open the door to
a very similar modification, for another handful of processors, which
would be really quite out of hand IMO.


Now that the mips sim 'multi' bits are in place (including good
default), and we have MIPS_MACH(SD) (thanks! 8-), it should be
possible to code a simple macro which checks for the appropriate bfd
machine, and decides whether interlocks are present.

I'd rather see an implementation that acts somewhat like the (rough,
uncompiled, not sanity-checked) patch below.  Additional advice: make
sure the comment describing the new macros mentions the fact that they
should have cases only for certain ISAs' processors (mipsIV, mipsV).

The names were just a suggestion.  there are probably better, shorter
ones, and I didn't try to reconcile them with any other code (e.g. the
code in headers where they might be defined 8-).


This type of change has the "right" properties, IMO:

        * boils down to a no-op if not multi.

        * doesn't impact ISAs with no hazard ambiguity if multi.

        * shouldn't cause maint pain long term if everybody decides to
          make the code right for their fave MIPS IV arch.  8-)



cgd


[-- Attachment #2: rough_haz.diff --]
[-- Type: text/x-patch, Size: 2977 bytes --]

Index: mips.igen
===================================================================
RCS file: /cvs/src/src/sim/mips/mips.igen,v
retrieving revision 1.55
diff -u -p -r1.55 mips.igen
--- mips.igen	20 Jan 2004 07:06:14 -0000	1.55
+++ mips.igen	18 Mar 2004 17:54:49 -0000
@@ -242,10 +242,15 @@
 // enforced restrictions (2) and (3) for more ISAs and CPU types than
 // necessary.  Unfortunately, at least some MIPS IV and later parts'
 // documentation describes them as having these hazards (e.g. vr5000),
-// so they can't be removed for at leats MIPS IV.  MIPS V hasn't been
-// checked (since there are no known hardware implementations).
-// 
+// so they can't be removed for at leats MIPS IV.  (As of MIPS32 and MIPS64,
+// the hazards are definitely architected to not be required.)
+//
+// To accomodate implementations of particular ISAs which don't have the
+// hazards, for some ISAs we have a version of these helper functions which
+// calls a function to determine if the particular architecture has
+// a hazard.
 
+// 
 // check_mf_cycles:
 //
 // Helper used by check_mt_hilo, check_mult_hilo, and check_div_hilo
@@ -274,8 +279,6 @@
 *mipsI:
 *mipsII:
 *mipsIII:
-*mipsIV:
-*mipsV:
 *vr4100:
 *vr5000:
 {
@@ -287,6 +290,18 @@
 }
 
 :function:::int:check_mt_hilo:hilo_history *history
+*mipsIV:
+*mipsV:
+{
+  signed64 time = sim_events_time (SD);
+  int ok = (MIPS_ARCH_HAS_MT_HILO_HAZARD (SD_)
+	    || check_mf_cycles (SD_, history, time, "MT"));
+  history->mt.timestamp = time;
+  history->mt.cia = CIA;
+  return ok;
+}
+
+:function:::int:check_mt_hilo:hilo_history *history
 *mips32:
 *mips64:
 *r3900:
@@ -350,8 +365,6 @@
 *mipsI:
 *mipsII:
 *mipsIII:
-*mipsIV:
-*mipsV:
 *vr4100:
 *vr5000:
 {
@@ -366,6 +379,21 @@
 }
 
 :function:::int:check_mult_hilo:hilo_history *hi, hilo_history *lo
+*mipsIV:
+*mipsV:
+{
+  signed64 time = sim_events_time (SD);
+  int ok = (! MIPS_ARCH_HAS_MULT_HILO_HAZARD (SD_)
+	    || (check_mf_cycles (SD_, hi, time, "OP")
+	        && check_mf_cycles (SD_, lo, time, "OP")));
+  hi->op.timestamp = time;
+  lo->op.timestamp = time;
+  hi->op.cia = CIA;
+  lo->op.cia = CIA;
+  return ok;
+}
+
+:function:::int:check_mult_hilo:hilo_history *hi, hilo_history *lo
 *mips32:
 *mips64:
 *r3900:
@@ -389,8 +417,6 @@
 *mipsI:
 *mipsII:
 *mipsIII:
-*mipsIV:
-*mipsV:
 *vr4100:
 *vr5000:
 *r3900:
@@ -398,6 +424,21 @@
   signed64 time = sim_events_time (SD);
   int ok = (check_mf_cycles (SD_, hi, time, "OP")
 	    && check_mf_cycles (SD_, lo, time, "OP"));
+  hi->op.timestamp = time;
+  lo->op.timestamp = time;
+  hi->op.cia = CIA;
+  lo->op.cia = CIA;
+  return ok;
+}
+
+:function:::int:check_div_hilo:hilo_history *hi, hilo_history *lo
+*mipsIV:
+*mipsV:
+{
+  signed64 time = sim_events_time (SD);
+  int ok = (! MIPS_ARCH_HAS_DIV_HILO_HAZARD (SD_)
+	    || (check_mf_cycles (SD_, hi, time, "OP")
+	        && check_mf_cycles (SD_, lo, time, "OP")));
   hi->op.timestamp = time;
   lo->op.timestamp = time;
   hi->op.cia = CIA;