public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
From: Nelson Chu <nelson@rivosinc.com>
To: Tsukasa OI <research_trasio@irq.a4lg.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 19:12:58 +0800	[thread overview]
Message-ID: <CAPpQWtDJGWBxPPZ1-TYCf9rPVPfNFYO9w900GE5SczQ1Tn_rBw@mail.gmail.com> (raw)
In-Reply-To: <349dc99b-2e92-d1e7-0b9e-18c26f923d02@irq.a4lg.com>

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
> >>
> >

  reply	other threads:[~2022-10-06 11:13 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 [this message]
2022-10-06 12:08       ` Tsukasa OI

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=CAPpQWtDJGWBxPPZ1-TYCf9rPVPfNFYO9w900GE5SczQ1Tn_rBw@mail.gmail.com \
    --to=nelson@rivosinc.com \
    --cc=binutils@sourceware.org \
    --cc=kito.cheng@sifive.com \
    --cc=palmer@rivosinc.com \
    --cc=research_trasio@irq.a4lg.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).