From: Tsukasa OI <research_trasio@irq.a4lg.com>
To: Nelson Chu <nelson@rivosinc.com>
Cc: Binutils <binutils@sourceware.org>,
Kito Cheng <kito.cheng@sifive.com>,
Palmer Dabbelt <palmer@rivosinc.com>
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: <6f84c6ec-5b68-c2ac-3f45-3cd0ec1346da@irq.a4lg.com> (raw)
In-Reply-To: <CAPpQWtDJGWBxPPZ1-TYCf9rPVPfNFYO9w900GE5SczQ1Tn_rBw@mail.gmail.com>
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
>>>>
>>>
>
prev parent 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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6f84c6ec-5b68-c2ac-3f45-3cd0ec1346da@irq.a4lg.com \
--to=research_trasio@irq.a4lg.com \
--cc=binutils@sourceware.org \
--cc=kito.cheng@sifive.com \
--cc=nelson@rivosinc.com \
--cc=palmer@rivosinc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* 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).