Re: [PATCH 3/3] Adding more instructions to Allegrex CPU

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

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

> From: David Guillen Fandos <david@davidgf.net>
> 
> Many instructions exist already in MIPS32, however some have different
> encodings. Some are new (ie. wsbw, min, max) and might be very device

 Two spaces after the full stop here please.

> diff --git a/opcodes/mips-opc.c b/opcodes/mips-opc.c
> index 5dd6c8f51e..1c6cd567d4 100644
> --- a/opcodes/mips-opc.c
> +++ b/opcodes/mips-opc.c
> @@ -993,8 +993,10 @@ const struct mips_opcode mips_builtin_opcodes[] =
>  {"cins",		"t,r,+p,+S",	0x70000032, 0xfc00003f, WR_1|RD_2,		0,		IOCT,		0,	0 },
>  {"clo",			"d,s",		0x00000051, 0xfc1f07ff, WR_1|RD_2,		0,		I37,		0,	0 },
>  {"clo",			"U,s",		0x70000021, 0xfc0007ff, WR_1|RD_2,		0,		I32|N55,	0,	I37 },
> +{"clo",			"d,s",		0x00000017, 0xfc1f07ff, WR_1|RD_2,		0,		AL,		0,	0 },
>  {"clz",			"d,s",		0x00000050, 0xfc1f07ff, WR_1|RD_2,		0,		I37,		0,	0 },
>  {"clz",			"U,s",		0x70000020, 0xfc0007ff, WR_1|RD_2,		0,		I32|N55,	0,	I37 },
> +{"clz",			"d,s",		0x00000016, 0xfc1f07ff, WR_1|RD_2,		0,		AL,		0,	0 },

 As a lower ISA implementation please put these new entries first rather 
than last (similarly to "msub" and "msubu" below).

> @@ -2129,7 +2136,8 @@ const struct mips_opcode mips_builtin_opcodes[] =
>  {"addu_s.ob",		"d,s,t",	0x7c000114, 0xfc0007ff, WR_1|RD_2|RD_3,		0,		0,		D64,	0 },
>  {"addu_s.qb",		"d,s,t",	0x7c000110, 0xfc0007ff, WR_1|RD_2|RD_3,		0,		0,		D32,	0 },
>  {"addwc",		"d,s,t",	0x7c000450, 0xfc0007ff, WR_1|RD_2|RD_3,		0,		0,		D32,	0 },
> -{"bitrev",		"d,t",		0x7c0006d2, 0xffe007ff, WR_1|RD_2,		0,		0,		D32,	0 },
> +{"bitrev",		"d,w",		0x7c000520, 0xffe007ff, WR_1|RD_2,		0,		AL,		0,	0 },
> +{"bitrev",		"d,t",		0x7c0006d2, 0xffe007ff, WR_1|RD_2,		0,		0,		D32,	AL },

 Why is the AL exclusion actually needed with the second entry here?  I'd
have thought the DSP ASE requirement would keep the entry unavailable for 
Allegrex.

> diff --git a/gas/testsuite/gas/mips/allegrex.d b/gas/testsuite/gas/mips/allegrex.d
> new file mode 100644
> index 0000000000..89c2d092ad
> --- /dev/null
> +++ b/gas/testsuite/gas/mips/allegrex.d
> @@ -0,0 +1,49 @@
> +#as: -march=allegrex -mabi=32
> +#objdump: -dr --prefix-addresses --show-raw-insn -M reg-names=numeric
> +#name: Sony Allegrex CPU tests
> +
> +.*:     file format .*
> +
> +
> +Disassembly of section .text:
[...]
> +0x000000a0 7000003e 	dret

 This test case fails for some configurations due to a difference in 
section alignment between MIPS targets causing a different amount of 
padding to be added at the end to satisfy alignment:

extra lines in tmpdir/dump.out starting with "^ ...$"
EOF from .../gas/testsuite/gas/mips/allegrex.d
FAIL: Sony Allegrex CPU tests

One way to address this problem is to add padding by hand, such as with 
the patch below.  We use this across many test cases already.

 Otherwise this is OK.

  Maciej

---
 gas/testsuite/gas/mips/allegrex.d |    1 +
 gas/testsuite/gas/mips/allegrex.s |    3 +++
 2 files changed, 4 insertions(+)

Index: binutils-gdb/gas/testsuite/gas/mips/allegrex.d
===================================================================
--- binutils-gdb.orig/gas/testsuite/gas/mips/allegrex.d
+++ binutils-gdb/gas/testsuite/gas/mips/allegrex.d
@@ -47,3 +47,4 @@
 0x00000098 7002083d 	mfdr	\$2,\$1
 0x0000009c 7083083d 	mtdr	\$3,\$1
 0x000000a0 7000003e 	dret
+	\.\.\.
Index: binutils-gdb/gas/testsuite/gas/mips/allegrex.s
===================================================================
--- binutils-gdb.orig/gas/testsuite/gas/mips/allegrex.s
+++ binutils-gdb/gas/testsuite/gas/mips/allegrex.s
@@ -42,3 +42,6 @@
 	mtdr	$v1, $1
 	dret
 
+# Force some (non-delay-slot) zero bytes, to make 'objdump' print ...
+	.align	4, 0
+	.space	16