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

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

On Wed, 29 Mar 2023, david@davidgf.es wrote:

> From: David Guillen Fandos <david@davidgf.net>
> 
> Includes its corresponding gas tests.

 Please make the change description more elaborate, stating what the 
Allegrex CPU is (you've referred to the PSP in the cover letter, but that 
won't be recorded in the repository, so such a reference ought to appear 
here as well) and naming any relevant available sources of information, 
such as those used to create this work.  We'll need this to maintain this 
code.

> diff --git a/bfd/archures.c b/bfd/archures.c
> index ff1ee0f9de..775b2f4889 100644
> --- a/bfd/archures.c
> +++ b/bfd/archures.c
> @@ -173,6 +173,7 @@ DESCRIPTION
>  .#define bfd_mach_mips16000		16000
>  .#define bfd_mach_mips16		16
>  .#define bfd_mach_mips5			5
> +.#define bfd_mach_mips_allegrex		10111431 {* octal 'AL', 31 *}

 Please add a full stop here and make it two spaces afterwards...

> diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
> index b60ff960f0..a7f0930810 100644
> --- a/bfd/bfd-in2.h
> +++ b/bfd/bfd-in2.h
> @@ -1468,6 +1468,7 @@ enum bfd_architecture
>  #define bfd_mach_mips16000             16000
>  #define bfd_mach_mips16                16
>  #define bfd_mach_mips5                 5
> +#define bfd_mach_mips_allegrex         10111431 /* octal 'AL', 31 */

 ... and regenerate this accordingly.  This is as per the GNU Coding 
Standards: <https://www.gnu.org/prep/standards/standards.html#Formatting>.

> diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
> index 35bbd86044..64e64ee142 100644
> --- a/bfd/elfxx-mips.c
> +++ b/bfd/elfxx-mips.c
> @@ -7070,6 +7070,9 @@ _bfd_elf_mips_mach (flagword flags)
>      case E_MIPS_MACH_IAMR2:
>        return bfd_mach_mips_interaptiv_mr2;
>  
> +    case E_MIPS_MACH_ALLEGREX:
> +      return bfd_mach_mips_allegrex;
> +
>      default:
>        switch (flags & EF_MIPS_ARCH)
>  	{
> @@ -12438,6 +12441,10 @@ mips_set_isa_flags (bfd *abfd)
>        val = E_MIPS_ARCH_64 | E_MIPS_MACH_XLR;
>        break;
>  
> +    case bfd_mach_mips_allegrex:
> +      val = E_MIPS_ARCH_2 | E_MIPS_MACH_ALLEGREX;
> +      break;
> +

 These switches were meant to be sorted by the ISA level, though by now 
people have messed this up already.  Let's not continue the trend though, 
so please put the two new entries below corresponding `bfd_mach_mips4010' 
ones above.

> diff --git a/include/elf/mips.h b/include/elf/mips.h
> index e2c3868348..64cab7e6f8 100644
> --- a/include/elf/mips.h
> +++ b/include/elf/mips.h
> @@ -302,6 +302,7 @@ END_RELOC_NUMBERS (R_MIPS_maxext)
>  #define E_MIPS_MACH_GS464       0x00A20000
>  #define E_MIPS_MACH_GS464E	0x00A30000
>  #define E_MIPS_MACH_GS264E	0x00A40000
> +#define E_MIPS_MACH_ALLEGREX	0x00A50000

 Do you have any specific need for this particular E_MIPS_MACH value?  If 
not, then can you please leave the subsequent values here for Loongson 
folks to continue using?  There are numerous gaps in this allocation to 
fill and for a one-off allocation like this I'd suggest using one of them.  
E.g. 0x00840000.  Once we got this in place it'll be a part of the ABI and 
will have to stay there forever.

 There is a comment about Cygnus allocations above, but it's of historical 
value now.

> diff --git a/include/opcode/mips.h b/include/opcode/mips.h
> index 75d3fc257c..08b4399b0d 100644
> --- a/include/opcode/mips.h
> +++ b/include/opcode/mips.h
> @@ -1265,6 +1265,8 @@ static const unsigned int mips_isa_table[] = {
>  #define INSN_XLR                 0x00000020
>  /* Imagination interAptiv MR2.  */
>  #define INSN_INTERAPTIV_MR2	  0x04000000
> +/* Sony PSP Allegrex instruction.  */
> +#define INSN_ALLEGREX             0x08000000

 Please use tabs to align this.

> diff --git a/gas/config/tc-mips.c b/gas/config/tc-mips.c
> index e911aaa904..ee9362761d 100644
> --- a/gas/config/tc-mips.c
> +++ b/gas/config/tc-mips.c
> @@ -536,7 +537,7 @@ static int mips_32bitmode = 0;
>  
>  /* True, if CPU has support for ldc1 and sdc1. */
>  #define CPU_HAS_LDC1_SDC1(CPU)	\
> -   ((mips_opts.isa != ISA_MIPS1) && ((CPU) != CPU_R5900))
> +   ((mips_opts.isa != ISA_MIPS1) && ((CPU) != CPU_R5900) && ((CPU) != CPU_ALLEGREX))

 Please wrap the line to stay within 79 columns (74 is the preferred limit 
where feasible).  This also comes from the GNU Coding Standards.

> diff --git a/opcodes/mips-dis.c b/opcodes/mips-dis.c
> index 859d4e3806..742a47cb69 100644
> --- a/opcodes/mips-dis.c
> +++ b/opcodes/mips-dis.c
> @@ -706,6 +706,10 @@ const struct mips_arch_choice mips_arch_choices[] =
>      mips_cp0sel_names_xlr, ARRAY_SIZE (mips_cp0sel_names_xlr),
>      mips_cp1_names_mips3264, mips_hwr_names_numeric },
>  
> +  { "allegrex", 1, bfd_mach_mips_allegrex, CPU_ALLEGREX,
> +    ISA_MIPS2, 0, mips_cp0_names_numeric, NULL, 0,
> +    mips_cp1_names_numeric, mips_hwr_names_numeric },
> +

 This part of the array is for MIPSr1 and newer architecture processors 
(yes, someone put "loongson2e"/"loongson2f" entries out of order), so 
please put the entry for "allegrex" below "r4010" instead, which is where 
MIPS II entries reside.

> diff --git a/opcodes/mips-opc.c b/opcodes/mips-opc.c
> index 2a1a6cbb27..cfbcd0b9e1 100644
> --- a/opcodes/mips-opc.c
> +++ b/opcodes/mips-opc.c
> @@ -3408,11 +3411,11 @@ const struct mips_opcode mips_builtin_opcodes[] =
>  {"c0",			"C",		0x42000000, 0xfe000000,	CP,			0,		I1,		0,	IOCT|IOCTP|IOCT2 },
>  {"c1",			"C",		0x46000000, 0xfe000000,	FP_S,			0,		I1,		0,	0 },
>  {"c2",			"C",		0x4a000000, 0xfe000000,	CP,			0,		I1,		0,	N54|IOCT|IOCTP|IOCT2 },
> -{"c3",			"C",		0x4e000000, 0xfe000000,	CP,			0,		I1,		0,	I3_33 },
> +{"c3",			"C",		0x4e000000, 0xfe000000,	CP,			0,		I1,		0,	I3_33|AL },
>  {"cop0",		"C",		0,    (int) M_COP0,	INSN_MACRO,		0,		I1,		0,	IOCT|IOCTP|IOCT2 },
>  {"cop1",		"C",		0,    (int) M_COP1,	INSN_MACRO,		INSN2_M_FP_S,	I1,		0,	0 },
>  {"cop2",		"C",		0,    (int) M_COP2,	INSN_MACRO,		0,		I1,		0,	N54|IOCT|IOCTP|IOCT2 },
> -{"cop3",		"C",		0,    (int) M_COP3,	INSN_MACRO,		0,		I1,		0,	I3_33 },
> +{"cop3",		"C",		0,    (int) M_COP3,	INSN_MACRO,		0,		I1,		0,	I3_33|AL },

 Why do these generic "c3" and "cop3" encodings need to be removed for 
Allegrex?

> diff --git a/gas/testsuite/gas/mips/mips.exp b/gas/testsuite/gas/mips/mips.exp
> index e358c9716a..f28ea7e695 100644
> --- a/gas/testsuite/gas/mips/mips.exp
> +++ b/gas/testsuite/gas/mips/mips.exp
> @@ -536,6 +536,9 @@ mips_arch_create xlr 	64	mips64	{ oddspreg } \
>  mips_arch_create r5900 	64	mips3	{ gpr_ilocks singlefloat nollsc } \
>  			{ -march=r5900 -mtune=r5900 } { -mmips:5900 } \
>  			{ mipsr5900el-*-* mips64r5900el-*-* }
> +mips_arch_create allegrex 32	mips2	{ singlefloat oddspreg } \
> +			{ -march=allegrex -mtune=allegrex } { -mmips:allegrex } \

 Please put this below the entry for `r3900' to keep ISA level ordering.  
Also wrap the line to stay within 79 columns.

> +			{ allegrex-*-* allegrex-*-* }

 There's no `allegrex-*-*' configuration triplet added by your change (and 
I'd question it if there was), so please drop this part.

> @@ -1858,7 +1861,7 @@ if { [istarget mips*-*-vxworks*] } {
>      run_dump_test_arches "attr-gnu-4-0" "-mfp32 -32" \
>  				    [mips_arch_list_matching mips1 !mips32r6]
>      run_dump_test_arches "attr-gnu-4-0" "-mfpxx -32" \
> -				    [mips_arch_list_matching mips2 !r5900]
> +				    [mips_arch_list_matching mips2 !r5900 !allegrex]

 Please wrap the line to stay within 79 columns.  Likewise throughout the 
rest of the file.

> diff --git a/gas/testsuite/gas/mips/allegrex@isa-override-1.d b/gas/testsuite/gas/mips/allegrex@isa-override-1.d
> new file mode 100644
> index 0000000000..5d33ac45d7
> --- /dev/null
> +++ b/gas/testsuite/gas/mips/allegrex@isa-override-1.d
> @@ -0,0 +1,28 @@
> +#objdump: -dr --prefix-addresses --show-raw-insn
> +#name: MIPS ISA override code generation
> +#as: -32
> +
> +.*: +file format .*mips.*
> +
> +Disassembly of section \.text:
> +[0-9a-f]+ <[^>]*> 8c820000 	lw	v0,0\(a0\)
> +[0-9a-f]+ <[^>]*> 8c830004 	lw	v1,4\(a0\)
> +[0-9a-f]+ <[^>]*> 3c0189ab 	lui	at,0x89ab
> +[0-9a-f]+ <[^>]*> 00411025 	or	v0,v0,at
> +[0-9a-f]+ <[^>]*> dc820000 	0xdc820000
> +[0-9a-f]+ <[^>]*> 340189ab 	li	at,0x89ab
> +[0-9a-f]+ <[^>]*> 00010c38 	0x10c38
> +[0-9a-f]+ <[^>]*> 00411025 	or	v0,v0,at
> +[0-9a-f]+ <[^>]*> 3c029000 	lui	v0,0x9000
> +[0-9a-f]+ <[^>]*> 00021438 	0x21438
> +[0-9a-f]+ <[^>]*> 34428000 	ori	v0,v0,0x8000
> +[0-9a-f]+ <[^>]*> 00021438 	0x21438
> +[0-9a-f]+ <[^>]*> 8c820000 	lw	v0,0\(a0\)
> +[0-9a-f]+ <[^>]*> 8c830004 	lw	v1,4\(a0\)
> +[0-9a-f]+ <[^>]*> 3c0189ab 	lui	at,0x89ab
> +[0-9a-f]+ <[^>]*> 00411025 	or	v0,v0,at
> +[0-9a-f]+ <[^>]*> 8c820000 	lw	v0,0\(a0\)
> +[0-9a-f]+ <[^>]*> 8c830004 	lw	v1,4\(a0\)
> +[0-9a-f]+ <[^>]*> 3c0189ab 	lui	at,0x89ab
> +[0-9a-f]+ <[^>]*> 00411025 	or	v0,v0,at
> +	\.\.\.

 This was produced with outdated binutils and fails with current output:

regexp_diff match failure
regexp "^[0-9a-f]+ <[^>]*> dc820000 	0xdc820000$"
line   "00000010 <foo+0x10> dc820000 	.word	0xdc820000"
regexp_diff match failure
regexp "^[0-9a-f]+ <[^>]*> 00010c38 	0x10c38$"
line   "00000018 <foo+0x18> 00010c38 	.word	0x10c38"
regexp_diff match failure
regexp "^[0-9a-f]+ <[^>]*> 00021438 	0x21438$"
line   "00000024 <foo+0x24> 00021438 	.word	0x21438"
regexp_diff match failure
regexp "^[0-9a-f]+ <[^>]*> 00021438 	0x21438$"
line   "0000002c <foo+0x2c> 00021438 	.word	0x21438"
FAIL: MIPS ISA override code generation (allegrex)

Please update the dump accordingly, and always rebase and verify against 
the current upstream trunk before submitting.

 Have you actually been able to run regression testing of your change?  
If so, what target configuration did you use?

> diff --git a/gas/testsuite/gas/mips/allegrex@isa-override-1.s b/gas/testsuite/gas/mips/allegrex@isa-override-1.s
> new file mode 100644
> index 0000000000..02352f8554
> --- /dev/null
> +++ b/gas/testsuite/gas/mips/allegrex@isa-override-1.s
> @@ -0,0 +1,23 @@
> +	.text
> +	.globl	foo
> +	.ent	foo
> +foo:
> +	ld	$2, 0($4)
> +	or	$2, 0x89ab0000
> +	.set	push
> +	.set	mips3
> +	ld	$2, 0($4)
> +	or	$2, 0x89ab0000
> +	dli	$2, 0x9000000080000000
> +	.set	mips0
> +	ld	$2, 0($4)
> +	or	$2, 0x89ab0000
> +	.set	mips3
> +	.set	pop
> +	ld	$2, 0($4)
> +	or	$2, 0x89ab0000
> +	.end	foo
> +
> +# Force some (non-delay-slot) zero bytes, to make 'objdump' print ...
> +	.align	4, 0
> +	.space	16

 This is the same as gas/testsuite/gas/mips/r5900@isa-override-1.s.  
Please remove then and use:

#source: r5900@isa-override-1.s

in gas/testsuite/gas/mips/allegrex@isa-override-1.d instead.

  Maciej