public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
* [RFC PATCH 0/1] RISC-V: Common register pair framework
@ 2022-10-01  4:45 Tsukasa OI
  2022-10-01  4:45 ` [RFC PATCH 1/1] RISC-V: Implement common " Tsukasa OI
  2022-10-01  7:17 ` [RFC PATCH 0/1] RISC-V: Common " Nelson Chu
  0 siblings, 2 replies; 4+ messages in thread
From: Tsukasa OI @ 2022-10-01  4:45 UTC (permalink / raw)
  To: Tsukasa OI, Nelson Chu, Kito Cheng, Palmer Dabbelt; +Cc: binutils

Hi all RISC-V folks,

My 'Zfinx' fixes PATCH 3/3 is going to be revised due to:

(1) Lack of 'Zhinxmin' + 'Z[dq]inx' support
(2) Insufficient test coverage
(3) Possible room to improve maintainability

In this week, I worked on (1)+(2) and improvement worked well.  For (3), I
might able to provide another proposal.


Register pairs using GPRs (that require certain alignments) are not
exclusive to 'Z[dq]inx'.  We currently expect that following extensions use
aligned register pairs:

-   'Zdinx' (ratified)
-   'Zqinx' (once proposed but not ratified)
-   'Zbpbo'       (a part of 'P'-extension proposal)
    Only used for instruction aliases WEXT and WEXTI.
-   'Zpsfoperand' (a part of 'P'-extension proposal)
    For instance, a 'P'-extension proposal implementation
    <https://github.com/riscvarchive/riscv-binutils-gdb/pull/257>
    uses "nds_rdp", "nds_rsp" and "nds_rtp" for register pair operands.

Due to this, if we have a common framework to handle register pairs, they
will be a lot easier.  But, as long as following statement is acceptable:

    If an invalid / reserved encoding is disassembled,
    it might be disassembled as an instruction with invalid operands.

Take for example, RVE.

If an instruction (for use in the RV32E environment) has the encoding
"add x14, x15, x16", x16 is an invalid GPR for RVE because RVI has 16 GPRs
(x0-x15) instead of 32 (x0-x31).

The best disassembler result (for me) would be ".4byte 0x1078733", meaning
an instruction is not recognized.  However, this resolution is sometimes
hard and forces to use some buffering before printing.

The second best solution would be... "add x14, x15, invalid16" or something
like that.  "invalid16" is used so that the encoding is not valid on the
current situation (e.g. ELF attributes meaning RV32E).  Note that similar
solution "unknown" is already used by the RISC-V disassembler.

    .insn 0x0107e753

This is an invalid encoding of "fadd.s fa4, fa5, fa6" with a reserved
rounding mode (0b110).  Current GNU Binutils disassembles this word as:

    fadd.s fa4,fa5,fa6,unknown

"unknown" makes sense here.
Extending this idea looks a viable solution to me.


So, my big question is, is it acceptable to extend this idea?
Here's only a part of such cases:

-   RVE: invalid register number
-   Register Pairs (Z[dq]inx and Zpsfoperand): invalid register number
-   Shift amount (SHAMT): equals to or greater than XLEN



If the idea above is acceptable for RISC-V GNU toolchain developers, this
patchset provides common framework for register pairs both for 'Z[dq]inx'
and 'Zpsfoperand'.

Operand Format:
1.  'l' (stands for "length")
2.  One of the following:
    '1' for  32-bit data  (or less; though redundant, makes code readable)
    '2' for  64-bit data  (RV32: 2 registers)
    '4' for 128-bit data  (RV32: 4 registers, RV64: 2 registers)
3.  One of the following:
    'd' for RD
    's' for RS1
    't' for RS2
    'r' for RS3
    'u' for RS1 and RS2 (where RS1 == RS2)
    (note that a GPR is expected here, even on 'r' and 'u')
    (To be added later for 'P' extension proposal; 'Zbpbo' WEXT aliases):
        'F' for RS1 and RS3 (RV32 "l2F" only; RS1 is even and RS3==RS1+1)

For instance, "l2d" means a 64-bit width destination register operand.  On
RV32, it would require a register pair and the register number must be even.
On RV64, it represents a GPR with no alignment requirements.

When assembling, it indirectly raises an error for an invalid register.

When disassembling, it once accepts all register numbers but the output
depends on whether the register number is valid for register pair alignment
requirements:

-   If valid, regular GPR operand is printed.
    (Style: dis_style_register)
-   If not,   "invalid%d" (where %d is the register number) is printed.
    (Style: dis_style_text)

I confirmed that this is sufficient to implement 'Z[dq]inx' and
'Zpsfoperand' except old 'P'-proposal aliases WEXT and WEXTI (now
implemented as aliases of FSR and FSRI; "l2F" will be added later).

I would like to hear your thoughts.

Thanks,
Tsukasa




Appendix: About my new 'Z[dq]inx' register pair implementation
          based on this Framework

My original 'Z[dq]inx' register pair implementation intended to implement
the best solution for the disassembler.  In contrast, my new 'Z[dq]inx'
register pair validation based on this framework has following benefits:

1.  Although an error is indirectly generated (as before), it explicitly
    checks whether the register number is valid directly on the assembler.
2.  My previous implementation required custom match_opcode functions and
    required to make separate opcode entry as shown below (on Zdinx):
    -   'D'
    -   'Zdinx' (XLEN==32)
    -   'Zdinx' (XLEN==64)
    This makes the code maintainance harder.  New implementation still
    requires to split opcode entry but...
    -   'D'
    -   'Zdinx' (for all XLEN)
    Not only reducing the changes, it will improve maintainability.
3.  Operand length and type fields are adjacent.
    In contrast to my previous implementation (operand types and custom
    match_opcode function representing length for all operands),
    it's pretty easy to understand.

This my new attempt for 'Z[dq]inx' in development is available at:
<https://github.com/a4lg/binutils-gdb/tree/riscv-float-combined-2>




Tsukasa OI (1):
  RISC-V: Implement common register pair framework

 gas/config/tc-riscv.c | 72 +++++++++++++++++++++++++++++++++++++++++++
 opcodes/riscv-dis.c   | 31 +++++++++++++++++++
 2 files changed, 103 insertions(+)


base-commit: 06bed95d8d2bac94956509dfc1f223d00e51eafb
-- 
2.34.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [RFC PATCH 1/1] RISC-V: Implement common register pair framework
  2022-10-01  4:45 [RFC PATCH 0/1] RISC-V: Common register pair framework Tsukasa OI
@ 2022-10-01  4:45 ` Tsukasa OI
  2022-10-01  7:17 ` [RFC PATCH 0/1] RISC-V: Common " Nelson Chu
  1 sibling, 0 replies; 4+ messages in thread
From: Tsukasa OI @ 2022-10-01  4:45 UTC (permalink / raw)
  To: Tsukasa OI, Nelson Chu, Kito Cheng, Palmer Dabbelt; +Cc: binutils

This commit implements common framework for aligned register pairs
(using GPRs).  This is particularly useful on following extensions:

-   'Zdinx'
-   'Zqinx' (once proposed but not ratified yet)
-   'Zpsfoperand' (a part of 'P'-extension proposal)

New operand type format is shown below:

1.  'l' (stands for "length")
2.  One of the following:
    '1' for  32-bit data (or less), (RV32: 1 register,  RV64: 1 register)
    '2' for  64-bit data            (RV32: 2 registers, RV64: 1 register)
    '4' for 128-bit data            (RV32: 4 registers, RV64: 2 registers)
3.  One of the following:
    'd' for RD
    's' for RS1
    't' for RS2
    'r' for RS3
    'u' for RS1 and RS2 (where RS1 == RS2)

gas/ChangeLog:

	* config/tc-riscv.c (riscv_ip): Add handling for "l[124][dstru]".
	(validate_riscv_insn): Likewise.

opcodes/ChangeLog:

	* riscv-dis.c (print_insn_args): Add handling for "l[124][dstru]".
---
 gas/config/tc-riscv.c | 72 +++++++++++++++++++++++++++++++++++++++++++
 opcodes/riscv-dis.c   | 31 +++++++++++++++++++
 2 files changed, 103 insertions(+)

diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index bd8f65d94fd..5e6fca3de9f 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -1251,6 +1251,30 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
 	case 'z': break; /* Zero immediate.  */
 	case '[': break; /* Unused operand.  */
 	case ']': break; /* Unused operand.  */
+	case 'l': /* Register pairs.  */
+	  switch (*++oparg)
+	    {
+	    case '1':
+	    case '2':
+	    case '4':
+	      break;
+	    default:
+	      goto unknown_validate_operand;
+	    }
+	  switch (*++oparg)
+	    {
+	    case 'd': USE_BITS (OP_MASK_RD, OP_SH_RD); break;
+	    case 's': USE_BITS (OP_MASK_RS1, OP_SH_RS1); break;
+	    case 't': USE_BITS (OP_MASK_RS2, OP_SH_RS2); break;
+	    case 'r': USE_BITS (OP_MASK_RS3, OP_SH_RS3); break;
+	    case 'u': /* RS1 == RS2.  */
+	      USE_BITS (OP_MASK_RS1, OP_SH_RS1);
+	      USE_BITS (OP_MASK_RS2, OP_SH_RS2);
+	      break;
+	    default:
+	      goto unknown_validate_operand;
+	    }
+	  break;
 	case '0': break; /* AMO displacement, must to zero.  */
 	case '1': break; /* Relaxation operand.  */
 	case 'F': /* Funct for .insn directive.  */
@@ -3021,6 +3045,54 @@ riscv_ip (char *str, struct riscv_cl_insn *ip, expressionS *imm_expr,
 		}
 	      break;
 
+	    case 'l': /* Register pairs.  */
+	      if (reg_lookup (&asarg, RCLASS_GPR, &regno))
+		{
+		  if (*asarg == ' ')
+		    ++asarg;
+
+		  unsigned dlen;
+		  switch (*++oparg)
+		    {
+		    case '1': dlen =  32; break;
+		    case '2': dlen =  64; break;
+		    case '4': dlen = 128; break;
+		    default:
+		      goto unknown_riscv_ip_operand;
+		    }
+		  char c = *++oparg;
+
+		  /* Check whether the register number is aligned properly.  */
+		  if (!(xlen >= dlen || (regno % (dlen / xlen)) == 0))
+		    break;
+
+		  /* Now the register number is valid.  Insert the number to
+		     the corresponding field(s).  */
+		  switch (c)
+		    {
+		    case 'd':
+		      INSERT_OPERAND (RD, *ip, regno);
+		      break;
+		    case 's':
+		      INSERT_OPERAND (RS1, *ip, regno);
+		      break;
+		    case 't':
+		      INSERT_OPERAND (RS2, *ip, regno);
+		      break;
+		    case 'r':
+		      INSERT_OPERAND (RS3, *ip, regno);
+		      break;
+		    case 'u':
+		      INSERT_OPERAND (RS1, *ip, regno);
+		      INSERT_OPERAND (RS2, *ip, regno);
+		      break;
+		    default:
+		      goto unknown_riscv_ip_operand;
+		    }
+		  continue;
+		}
+	      break;
+
 	    case 'D': /* Floating point RD.  */
 	    case 'S': /* Floating point RS1.  */
 	    case 'T': /* Floating point RS2.  */
diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 6ac69490b78..85b79db606c 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -488,6 +488,37 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info
 	  print (info->stream, dis_style_register, "%s", riscv_gpr_names[0]);
 	  break;
 
+	case 'l':
+	  {
+	    unsigned dlen;
+	    int regno;
+	    switch (*++oparg)
+	      {
+	      case '1': dlen =  32; break;
+	      case '2': dlen =  64; break;
+	      case '4': dlen = 128; break;
+	      default:
+		goto undefined_modifier;
+	      }
+	    switch (*++oparg)
+	      {
+	      case 'd': regno = rd; break;
+	      case 's': regno = rs1; break;
+	      case 't': regno = EXTRACT_OPERAND (RS2, l); break;
+	      case 'r': regno = EXTRACT_OPERAND (RS3, l); break;
+	      case 'u': regno = rs1; break; /* RS1 == RS2.  */
+	      default:
+		goto undefined_modifier;
+	      }
+	    /* Check whether the register number is aligned properly.  */
+	    if (xlen >= dlen || (regno % (dlen / xlen)) == 0)
+	      print (info->stream, dis_style_register, "%s",
+		     riscv_gpr_names[regno]);
+	    else
+	      print (info->stream, dis_style_text, "invalid%d", regno);
+	    break;
+	  }
+
 	case '>':
 	  print (info->stream, dis_style_immediate, "0x%x",
 		 (int)EXTRACT_OPERAND (SHAMT, l));
-- 
2.34.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC PATCH 0/1] RISC-V: Common register pair framework
  2022-10-01  4:45 [RFC PATCH 0/1] RISC-V: Common register pair framework Tsukasa OI
  2022-10-01  4:45 ` [RFC PATCH 1/1] RISC-V: Implement common " Tsukasa OI
@ 2022-10-01  7:17 ` Nelson Chu
  2022-10-06 12:21   ` Tsukasa OI
  1 sibling, 1 reply; 4+ messages in thread
From: Nelson Chu @ 2022-10-01  7:17 UTC (permalink / raw)
  To: Tsukasa OI; +Cc: Kito Cheng, Palmer Dabbelt, binutils

On Sat, Oct 1, 2022 at 12:46 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote:
>
> Hi all RISC-V folks,
>
> My 'Zfinx' fixes PATCH 3/3 is going to be revised due to:
>
> (1) Lack of 'Zhinxmin' + 'Z[dq]inx' support
> (2) Insufficient test coverage
> (3) Possible room to improve maintainability
>
> In this week, I worked on (1)+(2) and improvement worked well.  For (3), I
> might able to provide another proposal.
>
>
> Register pairs using GPRs (that require certain alignments) are not
> exclusive to 'Z[dq]inx'.  We currently expect that following extensions use
> aligned register pairs:
>
> -   'Zdinx' (ratified)
> -   'Zqinx' (once proposed but not ratified)
> -   'Zbpbo'       (a part of 'P'-extension proposal)
>     Only used for instruction aliases WEXT and WEXTI.
> -   'Zpsfoperand' (a part of 'P'-extension proposal)
>     For instance, a 'P'-extension proposal implementation
>     <https://github.com/riscvarchive/riscv-binutils-gdb/pull/257>
>     uses "nds_rdp", "nds_rsp" and "nds_rtp" for register pair operands.
>
> Due to this, if we have a common framework to handle register pairs, they
> will be a lot easier.  But, as long as following statement is acceptable:
>
>     If an invalid / reserved encoding is disassembled,
>     it might be disassembled as an instruction with invalid operands.
>
> Take for example, RVE.
>
> If an instruction (for use in the RV32E environment) has the encoding
> "add x14, x15, x16", x16 is an invalid GPR for RVE because RVI has 16 GPRs
> (x0-x15) instead of 32 (x0-x31).
>
> The best disassembler result (for me) would be ".4byte 0x1078733", meaning
> an instruction is not recognized.  However, this resolution is sometimes
> hard and forces to use some buffering before printing.
>
> The second best solution would be... "add x14, x15, invalid16" or something
> like that.  "invalid16" is used so that the encoding is not valid on the
> current situation (e.g. ELF attributes meaning RV32E).  Note that similar
> solution "unknown" is already used by the RISC-V disassembler.
>
>     .insn 0x0107e753

The main purpose of .insn is to let users encode the (customer)
instructions which haven't been supported in assembler.  Therefore,
trying to recognize and report something for .insn is kind of
unnecessary, even if we can use it to encode the existing
instructions.  We should suggest users to stop writing the supported
instructions by .insn directives.

> This is an invalid encoding of "fadd.s fa4, fa5, fa6" with a reserved
> rounding mode (0b110).  Current GNU Binutils disassembles this word as:
>
>     fadd.s fa4,fa5,fa6,unknown
>
> "unknown" makes sense here.
> Extending this idea looks a viable solution to me.
>
>
> So, my big question is, is it acceptable to extend this idea?
> Here's only a part of such cases:
>
> -   RVE: invalid register number
> -   Register Pairs (Z[dq]inx and Zpsfoperand): invalid register number
> -   Shift amount (SHAMT): equals to or greater than XLEN
>
>
>
> If the idea above is acceptable for RISC-V GNU toolchain developers, this
> patchset provides common framework for register pairs both for 'Z[dq]inx'
> and 'Zpsfoperand'.
>
> Operand Format:
> 1.  'l' (stands for "length")
> 2.  One of the following:
>     '1' for  32-bit data  (or less; though redundant, makes code readable)
>     '2' for  64-bit data  (RV32: 2 registers)
>     '4' for 128-bit data  (RV32: 4 registers, RV64: 2 registers)
> 3.  One of the following:
>     'd' for RD
>     's' for RS1
>     't' for RS2
>     'r' for RS3
>     'u' for RS1 and RS2 (where RS1 == RS2)
>     (note that a GPR is expected here, even on 'r' and 'u')
>     (To be added later for 'P' extension proposal; 'Zbpbo' WEXT aliases):
>         'F' for RS1 and RS3 (RV32 "l2F" only; RS1 is even and RS3==RS1+1)
>
> For instance, "l2d" means a 64-bit width destination register operand.  On
> RV32, it would require a register pair and the register number must be even.
> On RV64, it represents a GPR with no alignment requirements.
>
> When assembling, it indirectly raises an error for an invalid register.

I would suggest you not to spend too much time on these topics about
error reporting.  I used to do something similar for rvv constraints,
but now all of them are abandoned, since the stricter the assembler
checks, the hardware test checking will fail,
https://github.com/riscvarchive/riscv-binutils-gdb/pull/193.
Unfortunately, there is no such thing as the best of both worlds.

Nelson

> When disassembling, it once accepts all register numbers but the output
> depends on whether the register number is valid for register pair alignment
> requirements:
>
> -   If valid, regular GPR operand is printed.
>     (Style: dis_style_register)
> -   If not,   "invalid%d" (where %d is the register number) is printed.
>     (Style: dis_style_text)
>
> I confirmed that this is sufficient to implement 'Z[dq]inx' and
> 'Zpsfoperand' except old 'P'-proposal aliases WEXT and WEXTI (now
> implemented as aliases of FSR and FSRI; "l2F" will be added later).
>
> I would like to hear your thoughts.
>
> Thanks,
> Tsukasa
>
>
>
>
> Appendix: About my new 'Z[dq]inx' register pair implementation
>           based on this Framework
>
> My original 'Z[dq]inx' register pair implementation intended to implement
> the best solution for the disassembler.  In contrast, my new 'Z[dq]inx'
> register pair validation based on this framework has following benefits:
>
> 1.  Although an error is indirectly generated (as before), it explicitly
>     checks whether the register number is valid directly on the assembler.
> 2.  My previous implementation required custom match_opcode functions and
>     required to make separate opcode entry as shown below (on Zdinx):
>     -   'D'
>     -   'Zdinx' (XLEN==32)
>     -   'Zdinx' (XLEN==64)
>     This makes the code maintainance harder.  New implementation still
>     requires to split opcode entry but...
>     -   'D'
>     -   'Zdinx' (for all XLEN)
>     Not only reducing the changes, it will improve maintainability.
> 3.  Operand length and type fields are adjacent.
>     In contrast to my previous implementation (operand types and custom
>     match_opcode function representing length for all operands),
>     it's pretty easy to understand.
>
> This my new attempt for 'Z[dq]inx' in development is available at:
> <https://github.com/a4lg/binutils-gdb/tree/riscv-float-combined-2>
>
>
>
>
> Tsukasa OI (1):
>   RISC-V: Implement common register pair framework
>
>  gas/config/tc-riscv.c | 72 +++++++++++++++++++++++++++++++++++++++++++
>  opcodes/riscv-dis.c   | 31 +++++++++++++++++++
>  2 files changed, 103 insertions(+)
>
>
> base-commit: 06bed95d8d2bac94956509dfc1f223d00e51eafb
> --
> 2.34.1
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC PATCH 0/1] RISC-V: Common register pair framework
  2022-10-01  7:17 ` [RFC PATCH 0/1] RISC-V: Common " Nelson Chu
@ 2022-10-06 12:21   ` Tsukasa OI
  0 siblings, 0 replies; 4+ messages in thread
From: Tsukasa OI @ 2022-10-06 12:21 UTC (permalink / raw)
  To: Nelson Chu, Binutils



On 2022/10/01 16:17, Nelson Chu wrote:
> On Sat, Oct 1, 2022 at 12:46 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote:
>>
>> Hi all RISC-V folks,
>>
>> My 'Zfinx' fixes PATCH 3/3 is going to be revised due to:
>>
>> (1) Lack of 'Zhinxmin' + 'Z[dq]inx' support
>> (2) Insufficient test coverage
>> (3) Possible room to improve maintainability
>>
>> In this week, I worked on (1)+(2) and improvement worked well.  For (3), I
>> might able to provide another proposal.
>>
>>
>> Register pairs using GPRs (that require certain alignments) are not
>> exclusive to 'Z[dq]inx'.  We currently expect that following extensions use
>> aligned register pairs:
>>
>> -   'Zdinx' (ratified)
>> -   'Zqinx' (once proposed but not ratified)
>> -   'Zbpbo'       (a part of 'P'-extension proposal)
>>     Only used for instruction aliases WEXT and WEXTI.
>> -   'Zpsfoperand' (a part of 'P'-extension proposal)
>>     For instance, a 'P'-extension proposal implementation
>>     <https://github.com/riscvarchive/riscv-binutils-gdb/pull/257>
>>     uses "nds_rdp", "nds_rsp" and "nds_rtp" for register pair operands.
>>
>> Due to this, if we have a common framework to handle register pairs, they
>> will be a lot easier.  But, as long as following statement is acceptable:
>>
>>     If an invalid / reserved encoding is disassembled,
>>     it might be disassembled as an instruction with invalid operands.
>>
>> Take for example, RVE.
>>
>> If an instruction (for use in the RV32E environment) has the encoding
>> "add x14, x15, x16", x16 is an invalid GPR for RVE because RVI has 16 GPRs
>> (x0-x15) instead of 32 (x0-x31).
>>
>> The best disassembler result (for me) would be ".4byte 0x1078733", meaning
>> an instruction is not recognized.  However, this resolution is sometimes
>> hard and forces to use some buffering before printing.
>>
>> The second best solution would be... "add x14, x15, invalid16" or something
>> like that.  "invalid16" is used so that the encoding is not valid on the
>> current situation (e.g. ELF attributes meaning RV32E).  Note that similar
>> solution "unknown" is already used by the RISC-V disassembler.
>>
>>     .insn 0x0107e753
> 
> The main purpose of .insn is to let users encode the (customer)
> instructions which haven't been supported in assembler.  Therefore,
> trying to recognize and report something for .insn is kind of
> unnecessary, even if we can use it to encode the existing
> instructions.  We should suggest users to stop writing the supported
> instructions by .insn directives.
> 
>> This is an invalid encoding of "fadd.s fa4, fa5, fa6" with a reserved
>> rounding mode (0b110).  Current GNU Binutils disassembles this word as:
>>
>>     fadd.s fa4,fa5,fa6,unknown
>>
>> "unknown" makes sense here.
>> Extending this idea looks a viable solution to me.
>>
>>
>> So, my big question is, is it acceptable to extend this idea?
>> Here's only a part of such cases:
>>
>> -   RVE: invalid register number
>> -   Register Pairs (Z[dq]inx and Zpsfoperand): invalid register number
>> -   Shift amount (SHAMT): equals to or greater than XLEN
>>
>>
>>
>> If the idea above is acceptable for RISC-V GNU toolchain developers, this
>> patchset provides common framework for register pairs both for 'Z[dq]inx'
>> and 'Zpsfoperand'.
>>
>> Operand Format:
>> 1.  'l' (stands for "length")
>> 2.  One of the following:
>>     '1' for  32-bit data  (or less; though redundant, makes code readable)
>>     '2' for  64-bit data  (RV32: 2 registers)
>>     '4' for 128-bit data  (RV32: 4 registers, RV64: 2 registers)
>> 3.  One of the following:
>>     'd' for RD
>>     's' for RS1
>>     't' for RS2
>>     'r' for RS3
>>     'u' for RS1 and RS2 (where RS1 == RS2)
>>     (note that a GPR is expected here, even on 'r' and 'u')
>>     (To be added later for 'P' extension proposal; 'Zbpbo' WEXT aliases):
>>         'F' for RS1 and RS3 (RV32 "l2F" only; RS1 is even and RS3==RS1+1)
>>
>> For instance, "l2d" means a 64-bit width destination register operand.  On
>> RV32, it would require a register pair and the register number must be even.
>> On RV64, it represents a GPR with no alignment requirements.
>>
>> When assembling, it indirectly raises an error for an invalid register.
> 
> I would suggest you not to spend too much time on these topics about
> error reporting.  I used to do something similar for rvv constraints,
> but now all of them are abandoned, since the stricter the assembler
> checks, the hardware test checking will fail,
> https://github.com/riscvarchive/riscv-binutils-gdb/pull/193.
> Unfortunately, there is no such thing as the best of both worlds.

I'm talking about compliance to the specification.  And hardware test
checking is not very relevant to the toolchain (not totally irrelevant
though).  Enlighten me what are you on about exactly.  From my current
understandings, you are not explaining any valid reasons _not_ doing
such checks.

I provided two viable PoCs (Z[dq]inx and 'P'-extension proposal).  Why
are you doing that?!

Tsukasa

> 
> Nelson
> 
>> When disassembling, it once accepts all register numbers but the output
>> depends on whether the register number is valid for register pair alignment
>> requirements:
>>
>> -   If valid, regular GPR operand is printed.
>>     (Style: dis_style_register)
>> -   If not,   "invalid%d" (where %d is the register number) is printed.
>>     (Style: dis_style_text)
>>
>> I confirmed that this is sufficient to implement 'Z[dq]inx' and
>> 'Zpsfoperand' except old 'P'-proposal aliases WEXT and WEXTI (now
>> implemented as aliases of FSR and FSRI; "l2F" will be added later).
>>
>> I would like to hear your thoughts.
>>
>> Thanks,
>> Tsukasa
>>
>>
>>
>>
>> Appendix: About my new 'Z[dq]inx' register pair implementation
>>           based on this Framework
>>
>> My original 'Z[dq]inx' register pair implementation intended to implement
>> the best solution for the disassembler.  In contrast, my new 'Z[dq]inx'
>> register pair validation based on this framework has following benefits:
>>
>> 1.  Although an error is indirectly generated (as before), it explicitly
>>     checks whether the register number is valid directly on the assembler.
>> 2.  My previous implementation required custom match_opcode functions and
>>     required to make separate opcode entry as shown below (on Zdinx):
>>     -   'D'
>>     -   'Zdinx' (XLEN==32)
>>     -   'Zdinx' (XLEN==64)
>>     This makes the code maintainance harder.  New implementation still
>>     requires to split opcode entry but...
>>     -   'D'
>>     -   'Zdinx' (for all XLEN)
>>     Not only reducing the changes, it will improve maintainability.
>> 3.  Operand length and type fields are adjacent.
>>     In contrast to my previous implementation (operand types and custom
>>     match_opcode function representing length for all operands),
>>     it's pretty easy to understand.
>>
>> This my new attempt for 'Z[dq]inx' in development is available at:
>> <https://github.com/a4lg/binutils-gdb/tree/riscv-float-combined-2>
>>
>>
>>
>>
>> Tsukasa OI (1):
>>   RISC-V: Implement common register pair framework
>>
>>  gas/config/tc-riscv.c | 72 +++++++++++++++++++++++++++++++++++++++++++
>>  opcodes/riscv-dis.c   | 31 +++++++++++++++++++
>>  2 files changed, 103 insertions(+)
>>
>>
>> base-commit: 06bed95d8d2bac94956509dfc1f223d00e51eafb
>> --
>> 2.34.1
>>
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-10-06 12:21 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-01  4:45 [RFC PATCH 0/1] RISC-V: Common register pair framework Tsukasa OI
2022-10-01  4:45 ` [RFC PATCH 1/1] RISC-V: Implement common " Tsukasa OI
2022-10-01  7:17 ` [RFC PATCH 0/1] RISC-V: Common " Nelson Chu
2022-10-06 12:21   ` Tsukasa OI

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).