From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-sender-0.a4lg.com (mail-sender.a4lg.com [153.120.152.154]) by sourceware.org (Postfix) with ESMTPS id C3152385842E for ; Thu, 6 Oct 2022 12:08:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C3152385842E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=irq.a4lg.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=irq.a4lg.com Received: from [127.0.0.1] (localhost [127.0.0.1]) by mail-sender-0.a4lg.com (Postfix) with ESMTPSA id 86528300089; Thu, 6 Oct 2022 12:08:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=irq.a4lg.com; s=2017s01; t=1665058121; bh=aIILzs7QtZNRsGpyw6otr8AZr5cCO6zSaKdeDgjV8ZI=; h=Message-ID:Date:Mime-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type:Content-Transfer-Encoding; b=Ljc94Qkp0pFsrIlyVtrzklr2m89pldkZE7NNloxfK4RZzMbO5fII6WzLOdd2xmcSs j47/0N2wBYs9Z9nKZHPkcS/PpBVKkxvG4Dg5TNYLw/Rhmxr1pDxUqlNvzB6FCAbzmP e5Du1ayCPUugubGf4brEhsqPqLUMpp1uDyWEizUw= Message-ID: <6f84c6ec-5b68-c2ac-3f45-3cd0ec1346da@irq.a4lg.com> Date: Thu, 6 Oct 2022 21:08:39 +0900 Mime-Version: 1.0 Subject: Re: [RFC PATCH 0/1] RISC-V: Implement "extension variants" for diagnostics Content-Language: en-US To: Nelson Chu Cc: Binutils , Kito Cheng , Palmer Dabbelt References: <349dc99b-2e92-d1e7-0b9e-18c26f923d02@irq.a4lg.com> From: Tsukasa OI In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-6.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 >>>> "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 >>>> >>> >