public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
* [RFC PATCH 0/1] RISC-V: Implement "extension variants" for diagnostics
@ 2022-10-01  5:27 Tsukasa OI
  2022-10-01  5:27 ` [RFC PATCH 1/1] RISC-V: Implement extension variants Tsukasa OI
  2022-10-01  7:21 ` [RFC PATCH 0/1] RISC-V: Implement "extension variants" for diagnostics Nelson Chu
  0 siblings, 2 replies; 6+ messages in thread
From: Tsukasa OI @ 2022-10-01  5:27 UTC (permalink / raw)
  To: Tsukasa OI, Nelson Chu, Kito Cheng, Palmer Dabbelt; +Cc: binutils

Hi all RISC-V folks,

GitHub tracker:
<https://github.com/a4lg/binutils-gdb/wiki/riscv_insn_ext_variants>

While investigating possible implementation of both 'Z[fdq]inx' and 'P'
implementations, I found something common (not just register pairs).

Here, this patch implements what I call "extension variants".
It was formerly a part of 'Zfinx' fixes (formerly the flag was named
"INSN_F_OR_X").

If there is a instruction with multiple variants with different requirements
and the assembler fails to parse all variants, there is a case that needs to
refer ALL variants to generate proper diagnostics.

If an instruction with "INSN_HAS_EXT_VARS" fails on all variants, the
assembler now has a chance to modify the instruction class for proper
diagnostics.  A typical use of this feature is to select wider instruction
class when necessary.


[Usage: CLZ instruction ('Zbpbo')]

To implement proposed 'Zbpbo' extension, updating instruction class for
CLZ instruction is not enough.  If we change CLZ instruction class from
"INSN_CLASS_ZBB" to "INSN_CLASS_ZBB_OR_ZBPBO", we will **mistakenly**
support that instruction on RV64_Zbpbo because CLZ on 'Zbpbo' is RV32-only.

Possible use with "INSN_HAS_EXT_VARS" is to define two variants of CLZ
instruction with that flag:

1.  'Zbb' (RV32/RV64)
2.  'Zbpbo' (RV32)

If both fails, we can widen the instruction class from "INSN_CLASS_ZBPBO"
to "INSN_CLASS_ZBB_OR_ZBPBO" if XLEN is 32.
After doing that, we will get following diagnostics:

-   On RV32: 'Zbb' or 'Zbpbo' is required
-   On RV64: 'Zbb' is required


[Usage: 'D'/'Zdinx' or 'Q'/'Zqinx']

Due to use of register pairs, we need to split 'D'/'Q' and 'Zdinx'/'Zqinx'
variants.  For instance, if parsing "fmin.d" fails on both 'D' and 'Zdinx'
variants, we have to require 'D' or 'Zdinx', not just 'Zdinx', the last
"fmin.d" variant in riscv_opcodes.

1.  'D'
2.  'Zdinx'

If both fails, we can widen the instruction class from "INSN_CLASS_ZDINX" to
"INSN_CLASS_D_OR_ZDINX".
After doing that, we will get diagnostics saying 'D' or 'Zdinx' is required.



Thanks,
Tsukasa




Tsukasa OI (1):
  RISC-V: Implement extension variants

 gas/config/tc-riscv.c  | 27 +++++++++++++++++++++++++--
 include/opcode/riscv.h |  5 +++++
 2 files changed, 30 insertions(+), 2 deletions(-)


base-commit: b4477c7f666bdeb7f8e998633c7b0cb62310b9ef
-- 
2.34.1


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

* [RFC PATCH 1/1] RISC-V: Implement extension variants
  2022-10-01  5:27 [RFC PATCH 0/1] RISC-V: Implement "extension variants" for diagnostics Tsukasa OI
@ 2022-10-01  5:27 ` Tsukasa OI
  2022-10-01  7:21 ` [RFC PATCH 0/1] RISC-V: Implement "extension variants" for diagnostics Nelson Chu
  1 sibling, 0 replies; 6+ messages in thread
From: Tsukasa OI @ 2022-10-01  5:27 UTC (permalink / raw)
  To: Tsukasa OI, Nelson Chu, Kito Cheng, Palmer Dabbelt; +Cc: binutils

If there is a instruction with multiple variants with different
requirements and the assembler fails to parse all variants, there is a case
that needs to refer ALL variants to generate proper diagnostics.

If an instruction with INSN_HAS_EXT_VARS fails on all variants, the
assembler now has a chance to modify the instruction class for proper
diagnostics.  A typical use of this feature is to select _wider_ instruction
class when necessary.

gas/ChangeLog:

	* config/tc-riscv.c (riscv_ip): Add empty INSN_HAS_EXT_VARS
	handling.

include/ChangeLog:

	* opcode/riscv.h (INSN_HAS_EXT_VARS): New.
---
 gas/config/tc-riscv.c  | 27 +++++++++++++++++++++++++--
 include/opcode/riscv.h |  5 +++++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index 5e6fca3de9f..f83b0ff3dad 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -2376,6 +2376,7 @@ riscv_ip (char *str, struct riscv_cl_insn *ip, expressionS *imm_expr,
   unsigned int regno;
   const struct percent_op_match *p;
   struct riscv_ip_error error;
+  enum riscv_insn_class insn_class;
   error.msg = "unrecognized opcode";
   error.statement = str;
   error.missing_ext = NULL;
@@ -2402,8 +2403,30 @@ riscv_ip (char *str, struct riscv_cl_insn *ip, expressionS *imm_expr,
 
       if (!riscv_multi_subset_supports (&riscv_rps_as, insn->insn_class))
 	{
-	  error.missing_ext = riscv_multi_subset_supports_ext (&riscv_rps_as,
-							       insn->insn_class);
+	  insn_class = insn->insn_class;
+	  if (insn->pinfo != INSN_MACRO && insn->pinfo & INSN_HAS_EXT_VARS)
+	    {
+	      /*  If an instruction is not supported for example, there is a
+		  case where some extensions (that is not related to the last
+		  "same name" variants) should be printed for diagnostics.
+
+		  For instance, if parsing "fmin.d" fails on both 'D' and
+		  'Zdinx' variants, we have to require 'D' or 'Zdinx', not just
+		  'Zdinx', the last "fmin.d" variant in riscv_opcodes.
+
+		  For instructions with INSN_HAS_EXT_VARS, we should rewrite the
+		  instruction class (from a specific one to a generic one)
+		  to provide proper error output.
+	      */
+	      switch (insn_class)
+		{
+		default:
+		  break;
+		}
+	    }
+	  if (!riscv_multi_subset_supports (&riscv_rps_as, insn_class))
+	    error.missing_ext = riscv_multi_subset_supports_ext (&riscv_rps_as,
+								 insn_class);
 	  continue;
 	}
 
diff --git a/include/opcode/riscv.h b/include/opcode/riscv.h
index dd2569f6d55..3cfe132babe 100644
--- a/include/opcode/riscv.h
+++ b/include/opcode/riscv.h
@@ -496,6 +496,11 @@ struct riscv_opcode
 #define INSN_8_BYTE		0x00000040
 #define INSN_16_BYTE		0x00000050
 
+/* Instruction has different entry that shares the name but differs
+   in extension requirements (extension variants).  Those instructions must be
+   taken care if we should print an "extension required" error.  */
+#define INSN_HAS_EXT_VARS	0x00000080
+
 /* Instruction is actually a macro.  It should be ignored by the
    disassembler, and requires special treatment by the assembler.  */
 #define INSN_MACRO		0xffffffff
-- 
2.34.1


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

* Re: [RFC PATCH 0/1] RISC-V: Implement "extension variants" for diagnostics
  2022-10-01  5:27 [RFC PATCH 0/1] RISC-V: Implement "extension variants" for diagnostics Tsukasa OI
  2022-10-01  5:27 ` [RFC PATCH 1/1] RISC-V: Implement extension variants Tsukasa OI
@ 2022-10-01  7:21 ` Nelson Chu
  2022-10-01  8:25   ` Tsukasa OI
  1 sibling, 1 reply; 6+ messages in thread
From: Nelson Chu @ 2022-10-01  7:21 UTC (permalink / raw)
  To: Tsukasa OI; +Cc: Kito Cheng, Palmer Dabbelt, binutils

On Sat, Oct 1, 2022 at 1:27 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote:
>
> Hi all RISC-V folks,
>
> GitHub tracker:
> <https://github.com/a4lg/binutils-gdb/wiki/riscv_insn_ext_variants>
>
> While investigating possible implementation of both 'Z[fdq]inx' and 'P'
> implementations, I found something common (not just register pairs).
>
> Here, this patch implements what I call "extension variants".
> It was formerly a part of 'Zfinx' fixes (formerly the flag was named
> "INSN_F_OR_X").
>
> If there is a instruction with multiple variants with different requirements
> and the assembler fails to parse all variants, there is a case that needs to
> refer ALL variants to generate proper diagnostics.
>
> If an instruction with "INSN_HAS_EXT_VARS" fails on all variants, the
> assembler now has a chance to modify the instruction class for proper
> diagnostics.  A typical use of this feature is to select wider instruction
> class when necessary.
>
>
> [Usage: CLZ instruction ('Zbpbo')]
>
> To implement proposed 'Zbpbo' extension, updating instruction class for
> CLZ instruction is not enough.  If we change CLZ instruction class from
> "INSN_CLASS_ZBB" to "INSN_CLASS_ZBB_OR_ZBPBO", we will **mistakenly**
> support that instruction on RV64_Zbpbo because CLZ on 'Zbpbo' is RV32-only.
>
> Possible use with "INSN_HAS_EXT_VARS" is to define two variants of CLZ
> instruction with that flag:
>
> 1.  'Zbb' (RV32/RV64)
> 2.  'Zbpbo' (RV32)

Will we write an extra entry for clz can resolve the problem?

{"clz",        32, INSN_CLASS_ZBPBO, ...
{"clz",        0, INSN_CLASS_ZBB, ...

Nelson

> If both fails, we can widen the instruction class from "INSN_CLASS_ZBPBO"
> to "INSN_CLASS_ZBB_OR_ZBPBO" if XLEN is 32.
> After doing that, we will get following diagnostics:
>
> -   On RV32: 'Zbb' or 'Zbpbo' is required
> -   On RV64: 'Zbb' is required
>
>
> [Usage: 'D'/'Zdinx' or 'Q'/'Zqinx']
>
> Due to use of register pairs, we need to split 'D'/'Q' and 'Zdinx'/'Zqinx'
> variants.  For instance, if parsing "fmin.d" fails on both 'D' and 'Zdinx'
> variants, we have to require 'D' or 'Zdinx', not just 'Zdinx', the last
> "fmin.d" variant in riscv_opcodes.
>
> 1.  'D'
> 2.  'Zdinx'
>
> If both fails, we can widen the instruction class from "INSN_CLASS_ZDINX" to
> "INSN_CLASS_D_OR_ZDINX".
> After doing that, we will get diagnostics saying 'D' or 'Zdinx' is required.
>
>
>
> Thanks,
> Tsukasa
>
>
>
>
> Tsukasa OI (1):
>   RISC-V: Implement extension variants
>
>  gas/config/tc-riscv.c  | 27 +++++++++++++++++++++++++--
>  include/opcode/riscv.h |  5 +++++
>  2 files changed, 30 insertions(+), 2 deletions(-)
>
>
> base-commit: b4477c7f666bdeb7f8e998633c7b0cb62310b9ef
> --
> 2.34.1
>

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

* Re: [RFC PATCH 0/1] RISC-V: Implement "extension variants" for diagnostics
  2022-10-01  7:21 ` [RFC PATCH 0/1] RISC-V: Implement "extension variants" for diagnostics Nelson Chu
@ 2022-10-01  8:25   ` Tsukasa OI
  2022-10-06 11:12     ` Nelson Chu
  0 siblings, 1 reply; 6+ messages in thread
From: Tsukasa OI @ 2022-10-01  8:25 UTC (permalink / raw)
  To: Nelson Chu; +Cc: Binutils

On 2022/10/01 16:21, Nelson Chu wrote:
> On Sat, Oct 1, 2022 at 1:27 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote:
>>
>> Hi all RISC-V folks,
>>
>> GitHub tracker:
>> <https://github.com/a4lg/binutils-gdb/wiki/riscv_insn_ext_variants>
>>
>> While investigating possible implementation of both 'Z[fdq]inx' and 'P'
>> implementations, I found something common (not just register pairs).
>>
>> Here, this patch implements what I call "extension variants".
>> It was formerly a part of 'Zfinx' fixes (formerly the flag was named
>> "INSN_F_OR_X").
>>
>> If there is a instruction with multiple variants with different requirements
>> and the assembler fails to parse all variants, there is a case that needs to
>> refer ALL variants to generate proper diagnostics.
>>
>> If an instruction with "INSN_HAS_EXT_VARS" fails on all variants, the
>> assembler now has a chance to modify the instruction class for proper
>> diagnostics.  A typical use of this feature is to select wider instruction
>> class when necessary.
>>
>>
>> [Usage: CLZ instruction ('Zbpbo')]
>>
>> To implement proposed 'Zbpbo' extension, updating instruction class for
>> CLZ instruction is not enough.  If we change CLZ instruction class from
>> "INSN_CLASS_ZBB" to "INSN_CLASS_ZBB_OR_ZBPBO", we will **mistakenly**
>> support that instruction on RV64_Zbpbo because CLZ on 'Zbpbo' is RV32-only.
>>
>> Possible use with "INSN_HAS_EXT_VARS" is to define two variants of CLZ
>> instruction with that flag:
>>
>> 1.  'Zbb' (RV32/RV64)
>> 2.  'Zbpbo' (RV32)
> 
> Will we write an extra entry for clz can resolve the problem?
> 
> {"clz",        32, INSN_CLASS_ZBPBO, ...
> {"clz",        0, INSN_CLASS_ZBB, ...
> 
> Nelson

I initially thought so but it turned out that it cannot.  I learned it
when I splitted D/Zdinx and Q/Zqinx.

If an instruction entry is not matched because an extension is missing,
information to store missing extension is stored before trying the next
variant.  The point here is, this information is _overwritten_ when not
matched and only the last one is used to generate the error message when
all variants are not matched.

So (on environment with -march=rv32i with no 'Zbb' or 'Zbpbo'), just
placing two entries like that results in the message saying 'Zbb' is
required ('Zbpbo' is going to be ignored).

That's why something has to change (and I think that this is the one of
the simplest solution).

Thanks,
Tsukasa

> 
>> If both fails, we can widen the instruction class from "INSN_CLASS_ZBPBO"
>> to "INSN_CLASS_ZBB_OR_ZBPBO" if XLEN is 32.
>> After doing that, we will get following diagnostics:
>>
>> -   On RV32: 'Zbb' or 'Zbpbo' is required
>> -   On RV64: 'Zbb' is required
>>
>>
>> [Usage: 'D'/'Zdinx' or 'Q'/'Zqinx']
>>
>> Due to use of register pairs, we need to split 'D'/'Q' and 'Zdinx'/'Zqinx'
>> variants.  For instance, if parsing "fmin.d" fails on both 'D' and 'Zdinx'
>> variants, we have to require 'D' or 'Zdinx', not just 'Zdinx', the last
>> "fmin.d" variant in riscv_opcodes.
>>
>> 1.  'D'
>> 2.  'Zdinx'
>>
>> If both fails, we can widen the instruction class from "INSN_CLASS_ZDINX" to
>> "INSN_CLASS_D_OR_ZDINX".
>> After doing that, we will get diagnostics saying 'D' or 'Zdinx' is required.
>>
>>
>>
>> Thanks,
>> Tsukasa
>>
>>
>>
>>
>> Tsukasa OI (1):
>>   RISC-V: Implement extension variants
>>
>>  gas/config/tc-riscv.c  | 27 +++++++++++++++++++++++++--
>>  include/opcode/riscv.h |  5 +++++
>>  2 files changed, 30 insertions(+), 2 deletions(-)
>>
>>
>> base-commit: b4477c7f666bdeb7f8e998633c7b0cb62310b9ef
>> --
>> 2.34.1
>>
> 

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

* Re: [RFC PATCH 0/1] RISC-V: Implement "extension variants" for diagnostics
  2022-10-01  8:25   ` Tsukasa OI
@ 2022-10-06 11:12     ` Nelson Chu
  2022-10-06 12:08       ` Tsukasa OI
  0 siblings, 1 reply; 6+ messages in thread
From: Nelson Chu @ 2022-10-06 11:12 UTC (permalink / raw)
  To: Tsukasa OI; +Cc: Binutils, Kito Cheng, Palmer Dabbelt

Combining the INSN_CLASS will make things complicated and may need
lots of special combine rules in the future.  You probably can combine
INSN_CLASS_A and INSN_CLASS_B easier, but it is hard to combine
something like INSN_CLASS_A_OR_B, INSN_CLASS_C_AND_D, ... into one
CLASS, so if we choose your proposal, then we will need a lot of
special handlings in the future, and that's what I don't really hope
to see.  Generally, consider we have,

{ insn, INSN_CLASS_A, ... },
{ insn, INSN_CLASS_B, ... },
{ insn, INSN_CLASS_C, ... },
(may have more ... )

The current assembler just reports an error for the last entry.  The
checking rule is,
INSN_CLASS_A? -> invalid operands of A -> INSN_CLASS_B? -> invalid
operands of B -> INSN_CLASS_C? -> invalid operands of C

Too strict error report is a bit impractical, since it is difficult to
report accurately by the current mechanism.  So two cases,
1. Once any of INSN_CLASS_* is passed, just reporting "illegal
operands" should be enough.  Or you probably want to report more,
something like "illegal operands for extension A...", but in fact you
may have a chance to see that all ABC extensions (or both A and C) are
enabled, so which one you want to report?  So that's why I said, just
reporting "illegal operands" is acceptable and correct.

2. we don't have any illegal operands since all of the INSN_CLASS* are
not passed.  Assume the strings of INSN_CLASS_A/B/C from
riscv_multi_subset_supports_ext are string_A/B/C, then you probably
can just report "extension string_A or string_B or string_C is/are
required".  Since we have the beginning entry of riscv_opcode from
str_hash_find, it should be easy to get the whole INSN_CLASS* and
their error string easier.

Anyway, I hope we can try the more general and compromise solutions
for every case, rather than make things too complicated and specially.


Thanks
Nelson

On Sat, Oct 1, 2022 at 4:26 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote:
>
> On 2022/10/01 16:21, Nelson Chu wrote:
> > On Sat, Oct 1, 2022 at 1:27 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote:
> >>
> >> Hi all RISC-V folks,
> >>
> >> GitHub tracker:
> >> <https://github.com/a4lg/binutils-gdb/wiki/riscv_insn_ext_variants>
> >>
> >> While investigating possible implementation of both 'Z[fdq]inx' and 'P'
> >> implementations, I found something common (not just register pairs).
> >>
> >> Here, this patch implements what I call "extension variants".
> >> It was formerly a part of 'Zfinx' fixes (formerly the flag was named
> >> "INSN_F_OR_X").
> >>
> >> If there is a instruction with multiple variants with different requirements
> >> and the assembler fails to parse all variants, there is a case that needs to
> >> refer ALL variants to generate proper diagnostics.
> >>
> >> If an instruction with "INSN_HAS_EXT_VARS" fails on all variants, the
> >> assembler now has a chance to modify the instruction class for proper
> >> diagnostics.  A typical use of this feature is to select wider instruction
> >> class when necessary.
> >>
> >>
> >> [Usage: CLZ instruction ('Zbpbo')]
> >>
> >> To implement proposed 'Zbpbo' extension, updating instruction class for
> >> CLZ instruction is not enough.  If we change CLZ instruction class from
> >> "INSN_CLASS_ZBB" to "INSN_CLASS_ZBB_OR_ZBPBO", we will **mistakenly**
> >> support that instruction on RV64_Zbpbo because CLZ on 'Zbpbo' is RV32-only.
> >>
> >> Possible use with "INSN_HAS_EXT_VARS" is to define two variants of CLZ
> >> instruction with that flag:
> >>
> >> 1.  'Zbb' (RV32/RV64)
> >> 2.  'Zbpbo' (RV32)
> >
> > Will we write an extra entry for clz can resolve the problem?
> >
> > {"clz",        32, INSN_CLASS_ZBPBO, ...
> > {"clz",        0, INSN_CLASS_ZBB, ...
> >
> > Nelson
>
> I initially thought so but it turned out that it cannot.  I learned it
> when I splitted D/Zdinx and Q/Zqinx.
>
> If an instruction entry is not matched because an extension is missing,
> information to store missing extension is stored before trying the next
> variant.  The point here is, this information is _overwritten_ when not
> matched and only the last one is used to generate the error message when
> all variants are not matched.
>
> So (on environment with -march=rv32i with no 'Zbb' or 'Zbpbo'), just
> placing two entries like that results in the message saying 'Zbb' is
> required ('Zbpbo' is going to be ignored).
>
> That's why something has to change (and I think that this is the one of
> the simplest solution).
>
> Thanks,
> Tsukasa
>
> >
> >> If both fails, we can widen the instruction class from "INSN_CLASS_ZBPBO"
> >> to "INSN_CLASS_ZBB_OR_ZBPBO" if XLEN is 32.
> >> After doing that, we will get following diagnostics:
> >>
> >> -   On RV32: 'Zbb' or 'Zbpbo' is required
> >> -   On RV64: 'Zbb' is required
> >>
> >>
> >> [Usage: 'D'/'Zdinx' or 'Q'/'Zqinx']
> >>
> >> Due to use of register pairs, we need to split 'D'/'Q' and 'Zdinx'/'Zqinx'
> >> variants.  For instance, if parsing "fmin.d" fails on both 'D' and 'Zdinx'
> >> variants, we have to require 'D' or 'Zdinx', not just 'Zdinx', the last
> >> "fmin.d" variant in riscv_opcodes.
> >>
> >> 1.  'D'
> >> 2.  'Zdinx'
> >>
> >> If both fails, we can widen the instruction class from "INSN_CLASS_ZDINX" to
> >> "INSN_CLASS_D_OR_ZDINX".
> >> After doing that, we will get diagnostics saying 'D' or 'Zdinx' is required.
> >>
> >>
> >>
> >> Thanks,
> >> Tsukasa
> >>
> >>
> >>
> >>
> >> Tsukasa OI (1):
> >>   RISC-V: Implement extension variants
> >>
> >>  gas/config/tc-riscv.c  | 27 +++++++++++++++++++++++++--
> >>  include/opcode/riscv.h |  5 +++++
> >>  2 files changed, 30 insertions(+), 2 deletions(-)
> >>
> >>
> >> base-commit: b4477c7f666bdeb7f8e998633c7b0cb62310b9ef
> >> --
> >> 2.34.1
> >>
> >

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

* Re: [RFC PATCH 0/1] RISC-V: Implement "extension variants" for diagnostics
  2022-10-06 11:12     ` Nelson Chu
@ 2022-10-06 12:08       ` Tsukasa OI
  0 siblings, 0 replies; 6+ messages in thread
From: Tsukasa OI @ 2022-10-06 12:08 UTC (permalink / raw)
  To: Nelson Chu; +Cc: Binutils, Kito Cheng, Palmer Dabbelt

On 2022/10/06 20:12, Nelson Chu wrote:
> Combining the INSN_CLASS will make things complicated and may need
> lots of special combine rules in the future.  You probably can combine
> INSN_CLASS_A and INSN_CLASS_B easier, but it is hard to combine
> something like INSN_CLASS_A_OR_B, INSN_CLASS_C_AND_D, ... into one
> CLASS, so if we choose your proposal, then we will need a lot of
> special handlings in the future, and that's what I don't really hope
> to see.  Generally, consider we have,
> 
> { insn, INSN_CLASS_A, ... },
> { insn, INSN_CLASS_B, ... },
> { insn, INSN_CLASS_C, ... },
> (may have more ... )
> 
> The current assembler just reports an error for the last entry.  The
> checking rule is,
> INSN_CLASS_A? -> invalid operands of A -> INSN_CLASS_B? -> invalid
> operands of B -> INSN_CLASS_C? -> invalid operands of C
> 
> Too strict error report is a bit impractical, since it is difficult to
> report accurately by the current mechanism.  So two cases,
> 1. Once any of INSN_CLASS_* is passed, just reporting "illegal
> operands" should be enough.  Or you probably want to report more,
> something like "illegal operands for extension A...", but in fact you
> may have a chance to see that all ABC extensions (or both A and C) are
> enabled, so which one you want to report?  So that's why I said, just
> reporting "illegal operands" is acceptable and correct.
> 
> 2. we don't have any illegal operands since all of the INSN_CLASS* are
> not passed.  Assume the strings of INSN_CLASS_A/B/C from
> riscv_multi_subset_supports_ext are string_A/B/C, then you probably
> can just report "extension string_A or string_B or string_C is/are
> required".  Since we have the beginning entry of riscv_opcode from
> str_hash_find, it should be easy to get the whole INSN_CLASS* and
> their error string easier.
> 
> Anyway, I hope we can try the more general and compromise solutions
> for every case, rather than make things too complicated and specially.

Case 2 is what I partially agree but case 1 is ABSOLUTELY NOT.

I admit that my proposal is not *very* extensible but I think this is
sufficient for a while.  I don't think my proposal will break in a few
years.  Long-term solution would be a list of instruction classes (and
optional failure reasons appended when not matched).  That's what you
pointed in the case 2, right?  I AM trying compromise mid-term solution
(to fix the problems WE EASILY FACE) and I don't think this is too
complicated and specialized.

> 
> 
> Thanks
> Nelson
> 
> On Sat, Oct 1, 2022 at 4:26 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote:
>>
>> On 2022/10/01 16:21, Nelson Chu wrote:
>>> On Sat, Oct 1, 2022 at 1:27 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote:
>>>>
>>>> Hi all RISC-V folks,
>>>>
>>>> GitHub tracker:
>>>> <https://github.com/a4lg/binutils-gdb/wiki/riscv_insn_ext_variants>
>>>>
>>>> While investigating possible implementation of both 'Z[fdq]inx' and 'P'
>>>> implementations, I found something common (not just register pairs).
>>>>
>>>> Here, this patch implements what I call "extension variants".
>>>> It was formerly a part of 'Zfinx' fixes (formerly the flag was named
>>>> "INSN_F_OR_X").
>>>>
>>>> If there is a instruction with multiple variants with different requirements
>>>> and the assembler fails to parse all variants, there is a case that needs to
>>>> refer ALL variants to generate proper diagnostics.
>>>>
>>>> If an instruction with "INSN_HAS_EXT_VARS" fails on all variants, the
>>>> assembler now has a chance to modify the instruction class for proper
>>>> diagnostics.  A typical use of this feature is to select wider instruction
>>>> class when necessary.
>>>>
>>>>
>>>> [Usage: CLZ instruction ('Zbpbo')]
>>>>
>>>> To implement proposed 'Zbpbo' extension, updating instruction class for
>>>> CLZ instruction is not enough.  If we change CLZ instruction class from
>>>> "INSN_CLASS_ZBB" to "INSN_CLASS_ZBB_OR_ZBPBO", we will **mistakenly**
>>>> support that instruction on RV64_Zbpbo because CLZ on 'Zbpbo' is RV32-only.
>>>>
>>>> Possible use with "INSN_HAS_EXT_VARS" is to define two variants of CLZ
>>>> instruction with that flag:
>>>>
>>>> 1.  'Zbb' (RV32/RV64)
>>>> 2.  'Zbpbo' (RV32)
>>>
>>> Will we write an extra entry for clz can resolve the problem?
>>>
>>> {"clz",        32, INSN_CLASS_ZBPBO, ...
>>> {"clz",        0, INSN_CLASS_ZBB, ...
>>>
>>> Nelson
>>
>> I initially thought so but it turned out that it cannot.  I learned it
>> when I splitted D/Zdinx and Q/Zqinx.
>>
>> If an instruction entry is not matched because an extension is missing,
>> information to store missing extension is stored before trying the next
>> variant.  The point here is, this information is _overwritten_ when not
>> matched and only the last one is used to generate the error message when
>> all variants are not matched.
>>
>> So (on environment with -march=rv32i with no 'Zbb' or 'Zbpbo'), just
>> placing two entries like that results in the message saying 'Zbb' is
>> required ('Zbpbo' is going to be ignored).
>>
>> That's why something has to change (and I think that this is the one of
>> the simplest solution).
>>
>> Thanks,
>> Tsukasa
>>
>>>
>>>> If both fails, we can widen the instruction class from "INSN_CLASS_ZBPBO"
>>>> to "INSN_CLASS_ZBB_OR_ZBPBO" if XLEN is 32.
>>>> After doing that, we will get following diagnostics:
>>>>
>>>> -   On RV32: 'Zbb' or 'Zbpbo' is required
>>>> -   On RV64: 'Zbb' is required
>>>>
>>>>
>>>> [Usage: 'D'/'Zdinx' or 'Q'/'Zqinx']
>>>>
>>>> Due to use of register pairs, we need to split 'D'/'Q' and 'Zdinx'/'Zqinx'
>>>> variants.  For instance, if parsing "fmin.d" fails on both 'D' and 'Zdinx'
>>>> variants, we have to require 'D' or 'Zdinx', not just 'Zdinx', the last
>>>> "fmin.d" variant in riscv_opcodes.
>>>>
>>>> 1.  'D'
>>>> 2.  'Zdinx'
>>>>
>>>> If both fails, we can widen the instruction class from "INSN_CLASS_ZDINX" to
>>>> "INSN_CLASS_D_OR_ZDINX".
>>>> After doing that, we will get diagnostics saying 'D' or 'Zdinx' is required.
>>>>
>>>>
>>>>
>>>> Thanks,
>>>> Tsukasa
>>>>
>>>>
>>>>
>>>>
>>>> Tsukasa OI (1):
>>>>   RISC-V: Implement extension variants
>>>>
>>>>  gas/config/tc-riscv.c  | 27 +++++++++++++++++++++++++--
>>>>  include/opcode/riscv.h |  5 +++++
>>>>  2 files changed, 30 insertions(+), 2 deletions(-)
>>>>
>>>>
>>>> base-commit: b4477c7f666bdeb7f8e998633c7b0cb62310b9ef
>>>> --
>>>> 2.34.1
>>>>
>>>
> 

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

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

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-01  5:27 [RFC PATCH 0/1] RISC-V: Implement "extension variants" for diagnostics Tsukasa OI
2022-10-01  5:27 ` [RFC PATCH 1/1] RISC-V: Implement extension variants Tsukasa OI
2022-10-01  7:21 ` [RFC PATCH 0/1] RISC-V: Implement "extension variants" for diagnostics Nelson Chu
2022-10-01  8:25   ` Tsukasa OI
2022-10-06 11:12     ` Nelson Chu
2022-10-06 12:08       ` 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).