Re: [REVIEW ONLY 1/3] RISC-V: Add "XUN@S" operand type

[Palmer Dabbelt <palmer@dabbelt.com>]

On Mon, 28 Nov 2022 17:23:57 PST (-0800), binutils@sourceware.org wrote:
> From: Tsukasa OI <research_trasio@irq.a4lg.com>
>
> This is a variant of operand type "XuN@S" but when disassembling, it's
> printed as a hexadecimal number.
>
> The author intends to use this operand type on:
>
> -   Shift amount operands on 'P'-extension proposal's shift instructions
>     (to make them consistent with regular shift instructions)
> -   Landing pad label operand on the 'Zisslpcfi' extension proposal
>     (because they allow three different precision of landing pad label
>     [9, 17 and 25-bits] with up to three likely consecutive instructions
>     with 9, 8 and 8-bit immediates respectively, printing them as binary-
>     based will fit better to these instructions)
>
> gas/ChangeLog:
>
> 	* config/tc-riscv.c (validate_riscv_insn, riscv_ip): Add new
> 	operand type and its handling.
>
> opcodes/ChangeLog:
>
> 	* riscv-dis.c (print_insn_args): Print new operand type value
> 	as a hexadecimal number.
> ---
>  gas/config/tc-riscv.c | 2 ++
>  opcodes/riscv-dis.c   | 9 ++++++---
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> index 0682eb355241..b58b7bc0cb05 100644
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -1399,6 +1399,7 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
>  		case 's': /* 'XsN@S' ... N-bit signed immediate at bit S.  */
>  		  goto use_imm;
>  		case 'u': /* 'XuN@S' ... N-bit unsigned immediate at bit S.  */
> +		case 'U': /* 'XUN@S' ... same but disassembled as hex.  */
>  		  goto use_imm;
>  		use_imm:
>  		  n = strtol (oparg + 1, (char **)&oparg, 10);
> @@ -3437,6 +3438,7 @@ riscv_ip (char *str, struct riscv_cl_insn *ip, expressionS *imm_expr,
>  		      sign = true;
>  		      goto parse_imm;
>  		    case 'u': /* 'XuN@S' ... N-bit unsigned immediate at bit S.  */
> +		    case 'U': /* 'XUN@S' ... same but disassembled as hex.  */
>  		      sign = false;
>  		      goto parse_imm;
>  		    parse_imm:
> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
> index 0e1f3b4610aa..b3127dccb3e0 100644
> --- a/opcodes/riscv-dis.c
> +++ b/opcodes/riscv-dis.c
> @@ -587,8 +587,9 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info
>  	    size_t n;
>  	    size_t s;
>  	    bool sign;
> +	    char opch = *++oparg;
>
> -	    switch (*++oparg)
> +	    switch (opch)
>  	      {
>  		case 'l': /* Literal.  */
>  		  oparg++;
> @@ -603,6 +604,7 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info
>  		  sign = true;
>  		  goto print_imm;
>  		case 'u': /* 'XuN@S' ... N-bit unsigned immediate at bit S.  */
> +		case 'U': /* 'XUN@S' ... same but disassembled as hex.  */
>  		  sign = false;
>  		  goto print_imm;
>  		print_imm:
> @@ -613,8 +615,9 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info
>  		  oparg--;
>
>  		  if (!sign)
> -		    print (info->stream, dis_style_immediate, "%lu",
> -			   (unsigned long)EXTRACT_U_IMM (n, s, l));
> +		    print (info->stream, dis_style_immediate,
> +			   opch == 'U' ? "0x%lx" : "%lu",
> +			   (unsigned long) EXTRACT_U_IMM (n, s, l));
>  		  else
>  		    print (info->stream, dis_style_immediate, "%li",
>  			   (signed long)EXTRACT_S_IMM (n, s, l));

IMO we're being way too complicated with the immediate formats here.  
That was even true for the T-Head stuff, but this is worse.  If the ISA 
folks want to pretend they're not adding new instruction encoding 
formats that's their decision, but they are in practice so let's just 
treat them that way.

I was going to do that for the T-Head stuff, but after doing the sscanf 
cleanups to make it work I got lazy.  Those got dropped, though, so 
maybe it's time to just do this right?