Patchwork [v2] MIPS: MIPS32r2 FP MADD instruction set support

login
register
mail settings
Submitter Maciej W. Rozycki
Date July 16, 2013, 1:41 p.m.
Message ID <alpine.DEB.1.10.1307160205590.20590@tp.orcam.me.uk>
Download mbox | patch
Permalink /patch/259419/
State New
Headers show

Comments

Maciej W. Rozycki - July 16, 2013, 1:41 p.m.
On Wed, 27 Feb 2013, Maciej W. Rozycki wrote:

> > Maciej, in that case, the rest of the patch is OK for 4.9, thanks.
> 
>  I will apply in due course then, thanks for your review.

 Regrettably after further investigation I have realised the change I 
proposed inadvertently enables more than just the FP MADD instruction set.  
It also enables the FP indexed memory access instructions.  While that 
itself is not a bad change, it will better be discussed separately.

 Here's a new version that does not enable anything beyond the FP MADD 
instruction set.  While making this update I also noticed and fixed a 
place in mips_rtx_costs where ISA_HAS_FP4 was used where ISA_HAS_FP_MADD* 
should be.

 I have regression-tested this change with the mips-linux-gnu target and 
the mips32r2/o32 multilib.  I have also verified that the instructions 
affected were absent across the binaries produced by the testsuite before 
applying this change and present afterwards.  For some reason only MADD.S, 
MADD.D, MSUB.S and MSUB.D instructions were produced though -- it looks 
like none of NMADD.S, NMADD.D, NMSUB.S and NMSUB.D instructions has
coverage in our testsuite.

 I have also verified no FP indexed memory access instructions were 
produced whether with or without the patch applied.  And for safety I have 
also likewise checked the reciprocals that I'll handle separately as well.

 OK to apply?

2013-07-16  Maciej W. Rozycki  <macro@codesourcery.com>

	gcc/
	* config/mips/mips.h (ISA_HAS_FP4): Correct formatting.
	(ISA_HAS_FP_MADD4_MSUB4): Also enable for ISA_MIPS32R2.
	(ISA_HAS_NMADD4_NMSUB4): Remove the MODE argument; rewrite in 
	terms of ISA_HAS_FP4, and also enable for ISA_MIPS32R2.
	(ISA_HAS_NMADD3_NMSUB3): Remove the MODE argument.
	* config/mips/mips.c (mips_rtx_costs) <PLUS>: Check for 
	ISA_HAS_FP_MADD4_MSUB4 || ISA_HAS_FP_MADD3_MSUB3 rather than 
	ISA_HAS_FP4.
	<MINUS, NEG>: Update according to changes to ISA_HAS_NMADD4_NMSUB4 
	and ISA_HAS_NMADD3_NMSUB3.
	* config/mips/mips.md (nmadd4<mode>, nmadd3<mode>): Likewise.
	(nmadd4<mode>_fastmath, nmadd3<mode>_fastmath): Likewise.
	(nmsub4<mode>, nmsub3<mode>): Likewise.
	(nmsub4<mode>_fastmath, nmsub3<mode>_fastmath): Likewise.

  Maciej
Richard Sandiford - July 16, 2013, 6:51 p.m.
"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>  I have regression-tested this change with the mips-linux-gnu target and 
> the mips32r2/o32 multilib.  I have also verified that the instructions 
> affected were absent across the binaries produced by the testsuite before 
> applying this change and present afterwards.  For some reason only MADD.S, 
> MADD.D, MSUB.S and MSUB.D instructions were produced though -- it looks 
> like none of NMADD.S, NMADD.D, NMSUB.S and NMSUB.D instructions has
> coverage in our testsuite.

Hmm, can you double-check?  We have gcc.target/mips/nmadd-*, so if we're
not producing the instructions there then that sounds like a bug.

Thanks,
Richard
Maciej W. Rozycki - July 16, 2013, 7:41 p.m.
On Tue, 16 Jul 2013, Richard Sandiford wrote:

> >  I have regression-tested this change with the mips-linux-gnu target and 
> > the mips32r2/o32 multilib.  I have also verified that the instructions 
> > affected were absent across the binaries produced by the testsuite before 
> > applying this change and present afterwards.  For some reason only MADD.S, 
> > MADD.D, MSUB.S and MSUB.D instructions were produced though -- it looks 
> > like none of NMADD.S, NMADD.D, NMSUB.S and NMSUB.D instructions has
> > coverage in our testsuite.
> 
> Hmm, can you double-check?  We have gcc.target/mips/nmadd-*, so if we're
> not producing the instructions there then that sounds like a bug.

 I only checked executables, these tests do not produce any.  I didn't 
think of checking tests that do not produce executables, because they do 
not check run-time validity of code produced.  These three tests you've 
referred to all pass with or without the change, but they are tangential 
to it because they all force -mips4.  Sorry for being inaccurate.

  Maciej
Richard Sandiford - July 16, 2013, 8:05 p.m.
"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> On Tue, 16 Jul 2013, Richard Sandiford wrote:
>
>> >  I have regression-tested this change with the mips-linux-gnu target and 
>> > the mips32r2/o32 multilib.  I have also verified that the instructions 
>> > affected were absent across the binaries produced by the testsuite before 
>> > applying this change and present afterwards.  For some reason only MADD.S, 
>> > MADD.D, MSUB.S and MSUB.D instructions were produced though -- it looks 
>> > like none of NMADD.S, NMADD.D, NMSUB.S and NMSUB.D instructions has
>> > coverage in our testsuite.
>> 
>> Hmm, can you double-check?  We have gcc.target/mips/nmadd-*, so if we're
>> not producing the instructions there then that sounds like a bug.
>
>  I only checked executables, these tests do not produce any.  I didn't 
> think of checking tests that do not produce executables, because they do 
> not check run-time validity of code produced.  These three tests you've 
> referred to all pass with or without the change, but they are tangential 
> to it because they all force -mips4.

OK, as long as those tests pass then the patch is OK, thanks.  They aren't
tangential for a -march=vr5400 run though.  The tests force isa=4 rather
than -mips4, and since the VR5432 is a MIPS IV processor, the tests will
be run with that -march value.  E.g.

make check-gcc RUNTESTFLAGS="--target_board unix/-march=vr5400 mips.exp=nmadd*"

fails for me but:

make check-gcc RUNTESTFLAGS="--target_board unix/-march=vr5400/-mmad mips.exp=nmadd*"

passes.

Richard
Maciej W. Rozycki - July 16, 2013, 11:09 p.m.
On Tue, 16 Jul 2013, Richard Sandiford wrote:

> >  I only checked executables, these tests do not produce any.  I didn't 
> > think of checking tests that do not produce executables, because they do 
> > not check run-time validity of code produced.  These three tests you've 
> > referred to all pass with or without the change, but they are tangential 
> > to it because they all force -mips4.
> 
> OK, as long as those tests pass then the patch is OK, thanks.  They aren't
> tangential for a -march=vr5400 run though.  The tests force isa=4 rather
> than -mips4, and since the VR5432 is a MIPS IV processor, the tests will
> be run with that -march value.  E.g.
> 
> make check-gcc RUNTESTFLAGS="--target_board unix/-march=vr5400 mips.exp=nmadd*"
> 
> fails for me but:
> 
> make check-gcc RUNTESTFLAGS="--target_board unix/-march=vr5400/-mmad mips.exp=nmadd*"
> 
> passes.

 Thanks for correcting me on how isa=4 works in these tests.  Doesn't that 
mean they don't really provide correct coverage then?  I mean they should 
really pass on whetever targets support these instructions and shouldn't 
fail on targets that do not, either by being skipped or better yet by 
verifying that these instructions are not accidentally produced, shouldn't 
they?

 I have applied this change now, thanks for your review.

  Maciej

Patch

Index: gcc-fsf-trunk-quilt/gcc/config/mips/mips.c
===================================================================
--- gcc-fsf-trunk-quilt.orig/gcc/config/mips/mips.c	2013-07-13 00:59:53.000000000 +0100
+++ gcc-fsf-trunk-quilt/gcc/config/mips/mips.c	2013-07-13 01:24:21.590274806 +0100
@@ -3857,7 +3857,7 @@  mips_rtx_costs (rtx x, int code, int out
 
     case MINUS:
       if (float_mode_p
-	  && (ISA_HAS_NMADD4_NMSUB4 (mode) || ISA_HAS_NMADD3_NMSUB3 (mode))
+	  && (ISA_HAS_NMADD4_NMSUB4 || ISA_HAS_NMADD3_NMSUB3)
 	  && TARGET_FUSED_MADD
 	  && !HONOR_NANS (mode)
 	  && !HONOR_SIGNED_ZEROS (mode))
@@ -3890,7 +3890,7 @@  mips_rtx_costs (rtx x, int code, int out
 	{
 	  /* If this is part of a MADD or MSUB, treat the PLUS as
 	     being free.  */
-	  if (ISA_HAS_FP4
+	  if ((ISA_HAS_FP_MADD4_MSUB4 || ISA_HAS_FP_MADD3_MSUB3)
 	      && TARGET_FUSED_MADD
 	      && GET_CODE (XEXP (x, 0)) == MULT)
 	    *total = 0;
@@ -3909,7 +3909,7 @@  mips_rtx_costs (rtx x, int code, int out
 
     case NEG:
       if (float_mode_p
-	  && (ISA_HAS_NMADD4_NMSUB4 (mode) || ISA_HAS_NMADD3_NMSUB3 (mode))
+	  && (ISA_HAS_NMADD4_NMSUB4 || ISA_HAS_NMADD3_NMSUB3)
 	  && TARGET_FUSED_MADD
 	  && !HONOR_NANS (mode)
 	  && HONOR_SIGNED_ZEROS (mode))
Index: gcc-fsf-trunk-quilt/gcc/config/mips/mips.h
===================================================================
--- gcc-fsf-trunk-quilt.orig/gcc/config/mips/mips.h	2013-07-13 00:59:53.000000000 +0100
+++ gcc-fsf-trunk-quilt/gcc/config/mips/mips.h	2013-07-13 01:12:22.560918747 +0100
@@ -881,7 +881,7 @@  struct mips_cpu_info {
    FP madd and msub instructions, and the FP recip and recip sqrt
    instructions.  */
 #define ISA_HAS_FP4		((ISA_MIPS4				\
-				  || (ISA_MIPS32R2 && TARGET_FLOAT64)   \
+				  || (ISA_MIPS32R2 && TARGET_FLOAT64)	\
 				  || ISA_MIPS64				\
 				  || ISA_MIPS64R2)			\
 				 && !TARGET_MIPS16)
@@ -903,24 +903,20 @@  struct mips_cpu_info {
 #define GENERATE_MADD_MSUB	(TARGET_IMADD && !TARGET_MIPS16)
 
 /* ISA has floating-point madd and msub instructions 'd = a * b [+-] c'.  */
-#define ISA_HAS_FP_MADD4_MSUB4  ISA_HAS_FP4
+#define ISA_HAS_FP_MADD4_MSUB4  (ISA_HAS_FP4				\
+				 || (ISA_MIPS32R2 && !TARGET_MIPS16))
 
 /* ISA has floating-point madd and msub instructions 'c = a * b [+-] c'.  */
 #define ISA_HAS_FP_MADD3_MSUB3  TARGET_LOONGSON_2EF
 
 /* ISA has floating-point nmadd and nmsub instructions
    'd = -((a * b) [+-] c)'.  */
-#define ISA_HAS_NMADD4_NMSUB4(MODE)					\
-				((ISA_MIPS4				\
-				  || (ISA_MIPS32R2 && (MODE) == V2SFmode) \
-				  || ISA_MIPS64				\
-				  || ISA_MIPS64R2)			\
-				 && !TARGET_MIPS16)
+#define ISA_HAS_NMADD4_NMSUB4	(ISA_HAS_FP4				\
+				 || (ISA_MIPS32R2 && !TARGET_MIPS16))
 
 /* ISA has floating-point nmadd and nmsub instructions
    'c = -((a * b) [+-] c)'.  */
-#define ISA_HAS_NMADD3_NMSUB3(MODE)					\
-                                TARGET_LOONGSON_2EF
+#define ISA_HAS_NMADD3_NMSUB3	TARGET_LOONGSON_2EF
 
 /* ISA has count leading zeroes/ones instruction (not implemented).  */
 #define ISA_HAS_CLZ_CLO		((ISA_MIPS32				\
Index: gcc-fsf-trunk-quilt/gcc/config/mips/mips.md
===================================================================
--- gcc-fsf-trunk-quilt.orig/gcc/config/mips/mips.md	2013-07-13 00:59:53.000000000 +0100
+++ gcc-fsf-trunk-quilt/gcc/config/mips/mips.md	2013-07-13 01:00:40.529942011 +0100
@@ -2367,7 +2367,7 @@ 
 		   (mult:ANYF (match_operand:ANYF 1 "register_operand" "f")
 			      (match_operand:ANYF 2 "register_operand" "f"))
 		   (match_operand:ANYF 3 "register_operand" "f"))))]
-  "ISA_HAS_NMADD4_NMSUB4 (<MODE>mode)
+  "ISA_HAS_NMADD4_NMSUB4
    && TARGET_FUSED_MADD
    && HONOR_SIGNED_ZEROS (<MODE>mode)
    && !HONOR_NANS (<MODE>mode)"
@@ -2382,7 +2382,7 @@ 
 		   (mult:ANYF (match_operand:ANYF 1 "register_operand" "f")
 			      (match_operand:ANYF 2 "register_operand" "f"))
 		   (match_operand:ANYF 3 "register_operand" "0"))))]
-  "ISA_HAS_NMADD3_NMSUB3 (<MODE>mode)
+  "ISA_HAS_NMADD3_NMSUB3
    && TARGET_FUSED_MADD
    && HONOR_SIGNED_ZEROS (<MODE>mode)
    && !HONOR_NANS (<MODE>mode)"
@@ -2397,7 +2397,7 @@ 
 	 (mult:ANYF (neg:ANYF (match_operand:ANYF 1 "register_operand" "f"))
 		    (match_operand:ANYF 2 "register_operand" "f"))
 	 (match_operand:ANYF 3 "register_operand" "f")))]
-  "ISA_HAS_NMADD4_NMSUB4 (<MODE>mode)
+  "ISA_HAS_NMADD4_NMSUB4
    && TARGET_FUSED_MADD
    && !HONOR_SIGNED_ZEROS (<MODE>mode)
    && !HONOR_NANS (<MODE>mode)"
@@ -2412,7 +2412,7 @@ 
 	 (mult:ANYF (neg:ANYF (match_operand:ANYF 1 "register_operand" "f"))
 		    (match_operand:ANYF 2 "register_operand" "f"))
 	 (match_operand:ANYF 3 "register_operand" "0")))]
-  "ISA_HAS_NMADD3_NMSUB3 (<MODE>mode)
+  "ISA_HAS_NMADD3_NMSUB3
    && TARGET_FUSED_MADD
    && !HONOR_SIGNED_ZEROS (<MODE>mode)
    && !HONOR_NANS (<MODE>mode)"
@@ -2427,7 +2427,7 @@ 
 		   (mult:ANYF (match_operand:ANYF 2 "register_operand" "f")
 			      (match_operand:ANYF 3 "register_operand" "f"))
 		   (match_operand:ANYF 1 "register_operand" "f"))))]
-  "ISA_HAS_NMADD4_NMSUB4 (<MODE>mode)
+  "ISA_HAS_NMADD4_NMSUB4
    && TARGET_FUSED_MADD
    && HONOR_SIGNED_ZEROS (<MODE>mode)
    && !HONOR_NANS (<MODE>mode)"
@@ -2442,7 +2442,7 @@ 
 		   (mult:ANYF (match_operand:ANYF 2 "register_operand" "f")
 			      (match_operand:ANYF 3 "register_operand" "f"))
 		   (match_operand:ANYF 1 "register_operand" "0"))))]
-  "ISA_HAS_NMADD3_NMSUB3 (<MODE>mode)
+  "ISA_HAS_NMADD3_NMSUB3
    && TARGET_FUSED_MADD
    && HONOR_SIGNED_ZEROS (<MODE>mode)
    && !HONOR_NANS (<MODE>mode)"
@@ -2457,7 +2457,7 @@ 
 	 (match_operand:ANYF 1 "register_operand" "f")
 	 (mult:ANYF (match_operand:ANYF 2 "register_operand" "f")
 		    (match_operand:ANYF 3 "register_operand" "f"))))]
-  "ISA_HAS_NMADD4_NMSUB4 (<MODE>mode)
+  "ISA_HAS_NMADD4_NMSUB4
    && TARGET_FUSED_MADD
    && !HONOR_SIGNED_ZEROS (<MODE>mode)
    && !HONOR_NANS (<MODE>mode)"
@@ -2472,7 +2472,7 @@ 
 	 (match_operand:ANYF 1 "register_operand" "f")
 	 (mult:ANYF (match_operand:ANYF 2 "register_operand" "f")
 		    (match_operand:ANYF 3 "register_operand" "0"))))]
-  "ISA_HAS_NMADD3_NMSUB3 (<MODE>mode)
+  "ISA_HAS_NMADD3_NMSUB3
    && TARGET_FUSED_MADD
    && !HONOR_SIGNED_ZEROS (<MODE>mode)
    && !HONOR_NANS (<MODE>mode)"