Re: [PATCH] Add GINV(+VIRT) ASE for MIPSr6

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

On Thu, 18 May 2023, Tsing wrote:

> This patch adds the VZ extension to MIPSr6 as part of the GINV ASE.
> 
> gas/ChangeLog
> 	* config/tc-mips.c (mips_ases): Add microMIPS ASE.

 This surely isn't right.

> diff --git a/gas/testsuite/gas/mips/ginv-virt.d b/gas/testsuite/gas/mips/ginv-virt.d
> new file mode 100644
> index 00000000..50fa6d54
> --- /dev/null
> +++ b/gas/testsuite/gas/mips/ginv-virt.d
> @@ -0,0 +1,22 @@
> +#objdump: -pdr --prefix-addresses --show-raw-insn
> +#name: MIPS GINV Virtualization
> +#as: --defsym VX= -mginv -mvirt -32

 Please use `VX=1'.  I'm surprised using `--defsym' without giving the 
symbol a value even works.

> diff --git a/gas/testsuite/gas/mips/mips.exp b/gas/testsuite/gas/mips/mips.exp
> index fafed2dc..4e3e226f 100644
> --- a/gas/testsuite/gas/mips/mips.exp
> +++ b/gas/testsuite/gas/mips/mips.exp
> @@ -2131,6 +2131,7 @@ if { [istarget mips*-*-vxworks*] } {
>  
>      run_dump_test_arches "ginv"	[mips_arch_list_matching mips32r6]
>      run_dump_test_arches "ginv-err"	[mips_arch_list_matching mips32r6]
> +    run_dump_test_arches "ginv-virt"   [mips_arch_list_matching mips32r6]

 Use tabs for indentation.

> diff --git a/include/opcode/mips.h b/include/opcode/mips.h
> index 75d3fc25..b26a8ffd 100644
> --- a/include/opcode/mips.h
> +++ b/include/opcode/mips.h
> @@ -1316,6 +1316,9 @@ static const unsigned int mips_isa_table[] = {
>  /* The Enhanced VA Scheme (EVA) extension has instructions which are
>     only valid for the R6 ISA.  */
>  #define ASE_EVA_R6		0x02000000
> +/* The Virtualization ASE has Global INValidate (GINV)
> +   instructions which are only valid when both ASEs are enabled.  */
> +#define ASE_GINV_VIRT          0x08000000

 Why is there a hole in bit assignments here?  Also use tabs for 
indentation.

> @@ -2351,6 +2354,9 @@ extern const int bfd_mips16_num_opcodes;
>     "+*" 5-bit register vector element index at bit 16
>     "+|" 8-bit mask at bit 16
>  
> +   GINV ASE usage:
> +   "+\" 2 bit Global TLB invalidate type at bit 8

 We write bit-field references with a hyphen, i.e. "2-bit".

> diff --git a/opcodes/mips-opc.c b/opcodes/mips-opc.c
> index 2a1a6cbb..80a5124b 100644
> --- a/opcodes/mips-opc.c
> +++ b/opcodes/mips-opc.c
> @@ -412,6 +412,7 @@ decode_mips_operand (const char *p)
>  
>  /* Global INValidate (GINV) support.  */
>  #define GINV	ASE_GINV
> +#define GINVVZ ASE_GINV_VIRT

 Use a tab here.

> @@ -3329,6 +3330,7 @@ const struct mips_opcode mips_builtin_opcodes[] =
>  /* MIPS Global INValidate (GINV) ASE.  */
>  {"ginvi",		"s",		0x7c00003d, 0xfc1fffff, RD_1,			0,		0,		GINV,	0 },
>  {"ginvt",		"s,+\\",	0x7c0000bd, 0xfc1ffcff, RD_1,			0,		0,		GINV,	0 },
> +{"ginvgt",		"s,+\\",	0x7c0000fd, 0xfc1ffcff, RD_1,			0,		0,		GINVVZ,	0 },

 I take it the encoding is correct, as I can't find it in published docs, 
and the instruction itself is only mentioned once, in the context of SYNC.

 How did you verify your change?

  Maciej