Re: [PATCH v2 1/3] Add MIPS Allegrex CPU as a MIPS2-based CPU

["Maciej W. Rozycki" <macro@orcam.me.uk>]

On Wed, 14 Jun 2023, david@davidgf.es wrote:

> The Allegrex CPU was created by Sony Interactive Entertainment to power
> their portable console, the PlayStation Portable.
> The pspdev organization maintains all sorts of tools to create software
> for said device including documentation.

 I have still noticed a couple of minor issues as follows.

> diff --git a/gas/config/tc-mips.c b/gas/config/tc-mips.c
> index 71a046c2b6..3c42d59e5b 100644
> --- a/gas/config/tc-mips.c
> +++ b/gas/config/tc-mips.c
> @@ -535,8 +536,9 @@ static int mips_32bitmode = 0;
>  #define CPU_HAS_SEQ(CPU)	(CPU_IS_OCTEON (CPU))
>  
>  /* True, if CPU has support for ldc1 and sdc1. */
> -#define CPU_HAS_LDC1_SDC1(CPU)	\
> -   ((mips_opts.isa != ISA_MIPS1) && ((CPU) != CPU_R5900))
> +#define CPU_HAS_LDC1_SDC1(CPU) ((mips_opts.isa != ISA_MIPS1) && \
> +				((CPU) != CPU_R5900) && \
> +				((CPU) != CPU_ALLEGREX))

 I have reformatted this to follow the GNU Coding Standards (which require 
operators to put on the subsequent line when wrapping), and also replaced 
the separating space with a tab and aligned backslashes for consistency 
with the rest of this file.  I have also removed extraneous parentheses (I 
haven't flagged it before, but let's do it while redoing this macro 
anyway).  Last but not least I used my discretion to swap CPU_ALLEGREX 
with CPU_R5900 for consistency:

#define CPU_HAS_LDC1_SDC1(CPU)	(mips_opts.isa != ISA_MIPS1		\
				 && (CPU) != CPU_ALLEGREX		\
				 && (CPU) != CPU_R5900)

> diff --git a/gas/testsuite/gas/mips/mips.exp b/gas/testsuite/gas/mips/mips.exp
> index 0ea673906a..6dbf2e3746 100644
> --- a/gas/testsuite/gas/mips/mips.exp
> +++ b/gas/testsuite/gas/mips/mips.exp
> @@ -517,6 +517,9 @@ mips_arch_create r3000 	32	mips1	{} \
>  mips_arch_create r3900 	32	mips1	{ gpr_ilocks } \
>  			{ -march=r3900 -mtune=r3900 } { -mmips:3900 } \
>  			{ mipstx39-*-* mipstx39el-*-* }
> +mips_arch_create allegrex 	32	mips2	{ oddspreg singlefloat } \
> +			{ -march=allegrex -mtune=allegrex } \
> +			{ -mmips:allegrex }

 I have removed an extraneous tab that caused visible column misalignment,
which wasn't here in v1, hmm, and also fixed the order for `singlefloat':

mips_arch_create allegrex 32	mips2	{ singlefloat oddspreg } \
			{ -march=allegrex -mtune=allegrex } \
			{ -mmips:allegrex }

While at it I noticed the documentation for `singlefloat' isn't correct 
(it never was in the first place, but it's even less so now with Allegrex 
support added), so I have folded an obvious fix into your change.

> @@ -2081,13 +2093,15 @@ if { [istarget mips*-*-vxworks*] } {
>      run_list_test_arches "fpxx-oddfpreg" "-32 -mfpxx -mno-odd-spreg" \
>  			[mips_arch_list_matching mips2 !singlefloat]
>      run_dump_test_arches "fpxx-oddfpreg" \
> -			[mips_arch_list_matching oddspreg]
> +			[mips_arch_list_matching oddspreg !allegrex]
>      run_dump_test_arches "odd-spreg" "-mfp32" [mips_arch_list_matching oddspreg]
> -    run_dump_test_arches "odd-spreg" "-mfpxx" [mips_arch_list_matching oddspreg]
> +    run_dump_test_arches "odd-spreg" "-mfpxx" [mips_arch_list_matching \
> +						oddspreg !allegrex]
>      run_dump_test_arches "odd-spreg" "-mfp64" [mips_arch_list_matching mips32r2]
>      run_dump_test_arches "no-odd-spreg" "-mfp32" [mips_arch_list_matching mips1 \
>  							!mips32r6]
> -    run_dump_test_arches "no-odd-spreg" "-mfpxx" [mips_arch_list_matching mips2 !r5900]
> +    run_dump_test_arches "no-odd-spreg" "-mfpxx" [mips_arch_list_matching mips2 \
> +							!r5900 !allegrex]

 This still overruns 79 columns.  I have fixed this thus:

    run_dump_test_arches "no-odd-spreg" "-mfpxx" \
			[mips_arch_list_matching mips2 !r5900 !allegrex]

(and for consistency did the same with the change above; I'll fix other 
mess around here with a separate change).

 I have now committed this change, with the amendments mentioned.  Thank 
you for your contribution.

  Maciej