Message ID | alpine.DEB.1.10.1307160205590.20590@tp.orcam.me.uk |
---|---|
State | Accepted |
Headers | show |
"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
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
"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
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
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)"