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

Richard Sandiford
2004-03-28T10:16:53Z

cgd@broadcom.com writes:
> 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.

Hmm. I have something that claims to be the "MIPS IV Instruction Set",
Revision 3.2, dated September 1995, (C) MIPS Technologies. It _does_
document the hazards. Not quite sure where I got hold of it though...

If that's anything to go by, MIPS IV parts with these hazards might not
necessarily be broken. There seems to be enough ambiguity that the code
should just say it's ambiguous rather than call out one side as being wrong.

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

OK, this patch worked with a couple of minor mods: it needed s/SD_/SD/
in the macro calls, and there was some inverted logic in the check_mt_hilo
hunk. Not bad for an untested patch. ;)

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

I kept the same style of names, but with "MIPS_MACH_..." instead of
"MIPS_ARCH_...", for consistency with the existing MIPS_MACH macro.

As per request, I've added some test cases to the testsuite.
This involved splitting the existing model lists into two: those that
are supported directly, and those that can be modelled approximately
because they are "subsets" of directly-supported models. E.g.
mipsisa32-elf can simulate most mips1 code, but mips1 has the hazards
and mips32 doesn't.

Tested with the gcc testsute on mips64vrel-elf The VR5500 tests no longer
suffer from bogus hazard complaints. Also run through the sim testsuite
on mipsisa64-elf, mipsisa32-elf, mips64-elf, mips-elf and mips64vrel-elf.
OK to install?

Richard

2004-??-?? Chris Demetriou <cgd@broadcom.com>
Richard Sandiford <rsandifo@redhat.com>

sim/mips/
* sim-main.h (MIPS_MACH_HAS_MT_HILO_HAZARD)
(MIPS_MACH_HAS_MULT_HILO_HAZARD, MIPS_MACH_HAS_DIV_HILO_HAZARD): New.
* mips.igen (check_mt_hilo, check_mult_hilo, check_div_hilo): Provide
separate implementations for mipsIV and mipsV. Use new macros to
determine whether the restrictions apply.

sim/testsuite/
* sim/mips/hilo-hazard-[123].s: New files.
* sim/mips/basic.exp (run_hilo_test): New procedure.
(models): Only list models that are included in the configuration.
(submodels): New variable, set to submodels of the above.
(mips64vr-*-elf, mips64vrel-*-elf): New configuration stanza.
Run hilo-hazard-[123].s.

Index: sim/mips/sim-main.h
===================================================================
RCS file: /cvs/src/src/sim/mips/sim-main.h,v
retrieving revision 1.25
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.25 sim-main.h
*** sim/mips/sim-main.h 5 Jan 2003 07:56:59 -0000 1.25
--- sim/mips/sim-main.h 28 Mar 2004 09:33:12 -0000
*************** #define MIPS_MACH(SD) mips_mach_multi(SD
*** 953,958 ****
--- 953,970 ----
#define MIPS_MACH(SD) MIPS_MACH_DEFAULT
#endif

+ /* Macros for determining whether a MIPS IV or MIPS V part is subject
+ to the hi/lo restrictions described in mips.igen. */
+
+ #define MIPS_MACH_HAS_MT_HILO_HAZARD(SD) \
+ (MIPS_MACH (SD) != bfd_mach_mips5500)
+
+ #define MIPS_MACH_HAS_MULT_HILO_HAZARD(SD) \
+ (MIPS_MACH (SD) != bfd_mach_mips5500)
+
+ #define MIPS_MACH_HAS_DIV_HILO_HAZARD(SD) \
+ (MIPS_MACH (SD) != bfd_mach_mips5500)
+
#if H_REVEALS_MODULE_P (SIM_MAIN_INLINE)
#include "sim-main.c"
#endif
Index: sim/mips/mips.igen
===================================================================
RCS file: /cvs/src/src/sim/mips/mips.igen,v
retrieving revision 1.55
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.55 mips.igen
*** sim/mips/mips.igen 20 Jan 2004 07:06:14 -0000 1.55
--- sim/mips/mips.igen 28 Mar 2004 09:33:12 -0000
*************** 000000,5.*,5.*,5.*,5.OP,000101:SPECIAL:3
*** 238,250 ****
// On the r3900, restriction (2) is not present, and restriction (3) is not
// present for multiplication.
//
! // For now this code is paranoid. Historically the simulator
! // 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).
! //

// check_mf_cycles:
//
--- 238,252 ----
// On the r3900, restriction (2) is not present, and restriction (3) is not
// present for multiplication.
//
! // Unfortunately, there seems to be some confusion about whether the last
! // two restrictions should apply to "MIPS IV" as well. One edition of
! // the MIPS IV ISA says they do, but references in later ISA documents
! // suggest they don't.
! //
! // In reality, some MIPS IV parts, such as the VR5000 and VR5400, do have
! // these restrictions, while others, like the VR5500, don't. To accomodate
! // such differences, the MIPS IV and MIPS V version of these helper functions
! // use auxillary routines to determine whether the restriction applies.

// check_mf_cycles:
//
*************** 000000,5.*,5.*,5.*,5.OP,000101:SPECIAL:3
*** 274,281 ****
*mipsI:
*mipsII:
*mipsIII:
- *mipsIV:
- *mipsV:
*vr4100:
*vr5000:
{
--- 276,281 ----
*************** 000000,5.*,5.*,5.*,5.OP,000101:SPECIAL:3
*** 287,292 ****
--- 287,304 ----
}

:function:::int:check_mt_hilo:hilo_history *history
+ *mipsIV:
+ *mipsV:
+ {
+ signed64 time = sim_events_time (SD);
+ int ok = (! MIPS_MACH_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:
*************** 000000,5.*,5.*,5.*,5.OP,000101:SPECIAL:3
*** 350,357 ****
*mipsI:
*mipsII:
*mipsIII:
- *mipsIV:
- *mipsV:
*vr4100:
*vr5000:
{
--- 362,367 ----
*************** 000000,5.*,5.*,5.*,5.OP,000101:SPECIAL:3
*** 366,371 ****
--- 376,396 ----
}

:function:::int:check_mult_hilo:hilo_history *hi, hilo_history *lo
+ *mipsIV:
+ *mipsV:
+ {
+ signed64 time = sim_events_time (SD);
+ int ok = (! MIPS_MACH_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:
*************** 000000,5.*,5.*,5.*,5.OP,000101:SPECIAL:3
*** 389,396 ****
*mipsI:
*mipsII:
*mipsIII:
- *mipsIV:
- *mipsV:
*vr4100:
*vr5000:
*r3900:
--- 414,419 ----
*************** 000000,5.*,5.*,5.*,5.OP,000101:SPECIAL:3
*** 398,403 ****
--- 421,441 ----
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_MACH_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;
Index: sim/testsuite/sim/mips/basic.exp
===================================================================
RCS file: /cvs/src/src/sim/testsuite/sim/mips/basic.exp,v
retrieving revision 1.1
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.1 basic.exp
*** sim/testsuite/sim/mips/basic.exp 26 Jan 2004 08:12:44 -0000 1.1
--- sim/testsuite/sim/mips/basic.exp 28 Mar 2004 09:33:12 -0000
***************
*** 6,26 ****
# than the compiler) can't necessarily find.
unset_currtarget_info ldscript

# Only test mips*-elf (e.g., no mips-linux), and only test if the target
# board really is a simulator (sim tests don't work on real HW).
if {[istarget mips*-elf] && [board_info target exists is_simulator]} {

if {[istarget mipsisa64*-elf]} {
! set models "mips1 mips2 mips3 mips4 mips32 mips64"
} elseif {[istarget mipsisa32*-elf]} {
! set models "mips1 mips2 mips32"
} elseif {[istarget mips64*-elf]} {
! set models "mips1 mips2 mips3"
} else {
# fall back to just testing mips1 code.
set models "mips1"
}
set cpu_option -march

! run_sim_test sanity.s $models
}
--- 6,65 ----
# than the compiler) can't necessarily find.
unset_currtarget_info ldscript

+ # Do "run_sim_test TESTFILE MODELS" for each combination of the
+ # mf{lo,hi} -> mult/div/mt{lo,hi} hazard described in mips.igen.
+ # Insert NOPS nops after the mflo or mfhi.
+ proc run_hilo_test {testfile models nops} {
+ foreach reg {lo hi} {
+ foreach insn "{mult\t\$4,\$4} {div\t\$0,\$4,\$4} {mt$reg\t\$4}" {
+ set contents ""
+ append contents "\t.macro hilo\n"
+ append contents "\tmf$reg\t\$4\n"
+ append contents "\t.rept\t$nops\n"
+ append contents "\tnop\n"
+ append contents "\t.endr\n"
+ append contents "\t$insn\n"
+ append contents "\t.endm"
+
+ verbose -log "HILO test:\n$contents"
+ set file [open hilo-hazard.inc w]
+ puts $file $contents
+ close $file
+
+ run_sim_test $testfile $models
+ }
+ }
+ }
+
+
# Only test mips*-elf (e.g., no mips-linux), and only test if the target
# board really is a simulator (sim tests don't work on real HW).
if {[istarget mips*-elf] && [board_info target exists is_simulator]} {

if {[istarget mipsisa64*-elf]} {
! set models "mips32 mips64"
! set submodels "mips1 mips2 mips3 mips4"
} elseif {[istarget mipsisa32*-elf]} {
! set models "mips32"
! set submodels "mips1 mips2"
! } elseif {[istarget mips64vr-*-elf] || [istarget mips64vrel-*-elf]} {
! set models "vr4100 vr4111 vr4120 vr5000 vr5400 vr5500"
! set submodels "mips1 mips2 mips3 mips4"
} elseif {[istarget mips64*-elf]} {
! set models "mips3"
! set submodels "mips1 mips2"
} else {
# fall back to just testing mips1 code.
set models "mips1"
+ set submodels ""
}
+ append submodels " " $models
set cpu_option -march

! run_sim_test sanity.s $submodels
! foreach nops {0 1} {
! run_hilo_test hilo-hazard-1.s $models $nops
! run_hilo_test hilo-hazard-2.s $models $nops
! }
! run_hilo_test hilo-hazard-3.s $models 2
}
*** /dev/null Tue Jun 17 23:06:41 2003
--- sim/testsuite/sim/mips/hilo-hazard-1.s Sat Mar 27 20:56:32 2004
***************
*** 0 ****
--- 1,19 ----
+ # Test for architectures with mf{hi,lo} -> mult/div/mt{hi,lo} hazards.
+ #
+ # mach: mips1 mips2 mips3 mips4 vr4100 vr4111 vr4120 vr5000 vr5400
+ # as: -mabi=eabi
+ # ld: -N -Ttext=0x80010000
+ # output: HILO: * too close to MF at *\\n\\nprogram stopped*\\n
+ # xerror:
+
+ .include "hilo-hazard.inc"
+ .include "testutils.inc"
+
+ setup
+
+ .set noreorder
+ .ent DIAG
+ DIAG:
+ hilo
+ pass
+ .end DIAG
*** /dev/null Tue Jun 17 23:06:41 2003
--- sim/testsuite/sim/mips/hilo-hazard-2.s Sat Mar 27 20:56:41 2004
***************
*** 0 ****
--- 1,18 ----
+ # Test for architectures without mf{hi,lo} -> mult/div/mt{hi,lo} hazards.
+ #
+ # mach: vr5500 mips32 mips64
+ # as: -mabi=eabi
+ # ld: -N -Ttext=0x80010000
+ # output: pass\\n
+
+ .include "hilo-hazard.inc"
+ .include "testutils.inc"
+
+ setup
+
+ .set noreorder
+ .ent DIAG
+ DIAG:
+ hilo
+ pass
+ .end DIAG
*** /dev/null Tue Jun 17 23:06:41 2003
--- sim/testsuite/sim/mips/hilo-hazard-3.s Sat Mar 27 20:56:59 2004
***************
*** 0 ****
--- 1,18 ----
+ # Test for mf{hi,lo} -> mult/div/mt{hi,lo} with 2 nops inbetween.
+ #
+ # mach: all
+ # as: -mabi=eabi
+ # ld: -N -Ttext=0x80010000
+ # output: pass\\n
+
+ .include "hilo-hazard.inc"
+ .include "testutils.inc"
+
+ setup
+
+ .set noreorder
+ .ent DIAG
+ DIAG:
+ hilo
+ pass
+ .end DIAG