public inbox for
 help / color / mirror / Atom feed
From: Tsukasa OI <>
To: Nelson Chu <>
Cc: Binutils <>,
	Kito Cheng <>,
	Palmer Dabbelt <>
Subject: Re: [RFC PATCH 0/1] RISC-V: Implement "extension variants" for diagnostics
Date: Thu, 6 Oct 2022 21:08:39 +0900	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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 <> wrote:
>> On 2022/10/01 16:21, Nelson Chu wrote:
>>> On Sat, Oct 1, 2022 at 1:27 PM Tsukasa OI <> wrote:
>>>> Hi all RISC-V folks,
>>>> GitHub tracker:
>>>> <>
>>>> 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
>>>> 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

      reply	other threads:[~2022-10-06 12:08 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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
2022-10-01  8:25   ` Tsukasa OI
2022-10-06 11:12     ` Nelson Chu
2022-10-06 12:08       ` Tsukasa OI [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).