[v2] MIPS: MIPS32r2 FP MADD instruction set support

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

"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