From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oo1-xc32.google.com (mail-oo1-xc32.google.com [IPv6:2607:f8b0:4864:20::c32]) by sourceware.org (Postfix) with ESMTPS id 11D5A3884568 for ; Thu, 6 Oct 2022 11:13:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 11D5A3884568 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=rivosinc.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=rivosinc.com Received: by mail-oo1-xc32.google.com with SMTP id u19-20020a4a9e93000000b004757198549cso1194516ook.0 for ; Thu, 06 Oct 2022 04:13:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20210112.gappssmtp.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=Mm1WgwnWGgU/k1dSdVBvy2VCrh2VNm83krRcnMD+ev0=; b=lFGwLJmvaWg9K69+Am59zptZEfzeee9uPty2AmdIErtw7VVlQhUDzhijbTS73tU2hc 7XpF7CfNurOVe3TN8d+6urhRPvbASkhvsL4qlR9Yfjey/V8Npgxj9U7M4wL6kn4BLh/D ohqauiqhKNqUwdB1V1J87w9+tbk27NY6zPMoPcO1gN6pL2Is18qvoYDQdC51XSPotOAj 9NyVQoOTkjlCIQSX0bU5Zn2/KRyz0akWTzXZS+zPWqlRBZq97iFhb+VLoo+9rp8A7IPI 9hkutmliIRuGNSvTNDudcV5aS7KySc1RBd9SzKaLEnzF2lVPlyzr34fYIePd4LQvqXLh VlUQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=Mm1WgwnWGgU/k1dSdVBvy2VCrh2VNm83krRcnMD+ev0=; b=3F1ynG/sMYeEi3eOm+cgD2Q5y+JnBqrv+ZG1gCXYLEHm3G5N6j89yZOBPOj3RTuh4A jN2Sc87BoplsCtJpmoWyNa8AUoOG18C8itveRwf9/8Rmm+ta7eiw8ay/C8sVtr03orKD 3dJOZRbBntCNTkR9uBh654TAqeR/gckSc6VQMFFdfg1cK/6Kwa8YDmSmBm2zVIQ4QNPJ 9TPNRmpCAusXK6FlFRfIovNR0JKlRM3b8fqFPaNYnO/pNYSnZGK4Z4DbNiXoSckm0CkS SUk1FkkcJFl2jDfqZA7fCYkUIVyUx08woz0GAdZXoSUE6+LMqGLLasjq/oTfGr/bMdkx pxkg== X-Gm-Message-State: ACrzQf1zb221HDAso1/SxAdZ8i3eSgVRD7je1Zq4cB/hh6hDtm32jJmP BY1jzLXSTy90h34H5PFG3BhP0UweD66p/L9bKkpFqw== X-Google-Smtp-Source: AMsMyM5yldCN0E+bunN7jg9+gbPOcbt3rzN0+0uEWhYJ3nWzAW72bGcGmmYd3MdiR6M/6j7ks0MCtFi/VKsxd/Mg3S0= X-Received: by 2002:a4a:2c96:0:b0:47f:8b62:15fb with SMTP id o144-20020a4a2c96000000b0047f8b6215fbmr1452225ooo.71.1665054789358; Thu, 06 Oct 2022 04:13:09 -0700 (PDT) MIME-Version: 1.0 References: <349dc99b-2e92-d1e7-0b9e-18c26f923d02@irq.a4lg.com> In-Reply-To: <349dc99b-2e92-d1e7-0b9e-18c26f923d02@irq.a4lg.com> From: Nelson Chu Date: Thu, 6 Oct 2022 19:12:58 +0800 Message-ID: Subject: Re: [RFC PATCH 0/1] RISC-V: Implement "extension variants" for diagnostics To: Tsukasa OI Cc: Binutils , Kito Cheng , Palmer Dabbelt Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,RCVD_IN_DNSWL_NONE,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: 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 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 > >> > >