Comments

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 <macro@codesourcery.com>
gcc/
* config/mips/mips.h (ISA_HAS_NMADD4_NMSUB4): Remove
TARGET_MIPS5400 checking.
Maciej
gcc-mips-mad-5400.diff