[PATCH] MIPS: Remove VR54xx NMADD.fmt/NMADD.fmt control breakage

["Maciej W. Rozycki" <macro at codesourcery dot com>]

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