On Thu, 21 Feb 2013, Richard Sandiford wrote:
> > Finally, while at it, I found it interesting that we have separate
> > conditions to cover MADD.fmt/MSUB.fmt (ISA_HAS_FP_MADD4_MSUB4) and
> > NMADD.fmt/NMADD.fmt (ISA_HAS_NMADD4_NMSUB4) while all the four
> > instructions need to be implemented as a whole group per data format
> > supported and cannot be separated (the MIPS architecture specification
> > explicitly forbids subsetting). The difference between the two conditions
> > is the former expands to ISA_HAS_FP4, that is enables the subsubset for
> > any MIPS IV and up FPU while the latter has an extra "&& (!TARGET_MIPS5400
> > || TARGET_MAD)" qualifier.
> >
> > I went ahead and checked available NEC VR54xx documentation and here's
> > what I came up with:
> >
> > 1. "VR5400 MIPS RISC Microprocessor Family" datasheet (NEC doc #13362)
> > says:
> >
> > "The VR5400 processor family complies with the MIPS IV instruction set
> > and IEEE-754 floating-point and IEEE-1149.1/1149.1a JTAG specification,
> > [...]"
> >
> > 2. "VR5432 MIPS RISC Microprocessor User's Manual, Volume 2" (NEC doc
> > #13751) lists all the individual MADD.fmt, MSUB.fmt, NMADD.fmt and
> > NMSUB.fmt instructions in Chapter 18 "Floating-Point Unit Instruction
> > Set" with no restrictions as to their availability (the only other
> > member of the VR54xx family I know of is the VR5464 that is a
> > high-performance version of the VR5432 and is fully software
> > compatible).
> >
> > Further to that TARGET_MAD controls whether to "Use PMC-style 'mad'
> > instructions" that are all CPU rather than FPU instructions. The VR5432
> > indeed supports extra integer multiply-accumulate instructions, as
> > documented in #2 above; these are the MACC/MACCHI/MACCHIU/MACCU and
> > MSAC/MSACHI/MSACHIU/MSACU instructions as roughly covered by our
> > ISA_HAS_MACC, ISA_HAS_MSAC and ISA_HAS_MACCHI knobs (the latter is not
> > implied for TARGET_MIPS5400, perhaps because the family does not support
> > the doubleword variants).
> >
> > All in all it looks to me like a misplaced hunk. It was introduced in
> > rev. 56471 (you were named as one of the contributors on that commit, so
> > you may be able to remember and/or correct me if I am wrong here anywhere)
> > and it looks to me it should have been applied to the ISA_HAS_MADD_MSUB
> > macro instead that's still just a few lines above ISA_HAS_NMADD4_NMSUB4
> > (and was even closer to ISA_HAS_NMADD_NMSUB as the latter was then called;
> > the bodies were close enough back then for a hunk to apply cleanly to
> > either).
>
> I was named in that commit but the VR54xx stuff wasn't mine. I do remember
> that Mike put a lot of effort into tuning the VR54xx madd stuff though,
> because of the difficulty of having multiply-accumulate instructions
> that force the use of HI/LO on an architecture that also has efficient
> three-operand multiplies. So I'm pretty sure that this worked correctly
> in the Cygnus devo tree, and your explanation of a misplaced hunk seems
> very convincing.
Here's a change to remove this hunk as obviously inappropriate for
ISA_HAS_NMADD4_NMSUB4. Also AFAICT all integer multiply-accumulate
control for the VR54xx is done via the ISA_HAS_MACC and ISA_HAS_MSAC
rather than ISA_HAS_MADD_MSUB. The latter is only used to control MIPS
architecture multiply-accumulate instructions. See the `macc', `msac',
`mul_acc_si', `mul_sub_si', `<u>maddsidi4' and `<u>msubsidi4' patterns.
My conclusion therefore is there is no point in trying to fit this code to
ISA_HAS_MADD_MSUB, it's just not relevant there. It might go with
ISA_HAS_MACC and ISA_HAS_MSAC instead, but 11 years on I think it's simply
safer just to discard it entirely.
The hunk only seems to have slipped through, probably from an earlier
development version, because of its limited impact -- while disabling
NMADD.fmt and NMSUB.fmt for the VR54xx can result in a performance hit
it's by no means fatal.
I'm not sure how to test this change beyond making sure it builds (it
does, for the mips-linux-gnu target at least). I don't have a VR54xx
system available. OK to apply?
2013-07-16 Maciej W. Rozycki <ma...@codesourcery.com>
gcc/
* config/mips/mips.h (ISA_HAS_NMADD4_NMSUB4): Remove
TARGET_MIPS5400 checking.
Maciej
gcc-mips-mad-5400.diff
Index: gcc-fsf-trunk-quilt/gcc/config/mips/mips.h
===================================================================
--- gcc-fsf-trunk-quilt.orig/gcc/config/mips/mips.h 2013-07-13
00:13:02.000000000 +0100
+++ gcc-fsf-trunk-quilt/gcc/config/mips/mips.h 2013-07-13 00:47:48.490937673
+0100
@@ -915,7 +915,6 @@ struct mips_cpu_info {
|| (ISA_MIPS32R2 && (MODE) == V2SFmode) \
|| ISA_MIPS64 \
|| ISA_MIPS64R2) \
- && (!TARGET_MIPS5400 || TARGET_MAD) \
&& !TARGET_MIPS16)
/* ISA has floating-point nmadd and nmsub instructions
