From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-sender-0.a4lg.com (mail-sender-0.a4lg.com [IPv6:2401:2500:203:30b:4000:6bfe:4757:0]) by sourceware.org (Postfix) with ESMTPS id DBE323858C27 for ; Tue, 8 Feb 2022 09:51:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DBE323858C27 Message-ID: Date: Tue, 8 Feb 2022 18:51:28 +0900 Mime-Version: 1.0 Subject: Re: [PATCH 4/5] RISC-V: Relax `fmv.[sdq]' requirements Content-Language: en-US To: Palmer Dabbelt References: From: Tsukasa OI Cc: Binutils In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: binutils@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Binutils mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 08 Feb 2022 09:51:35 -0000 On 2022/02/08 11:00, Palmer Dabbelt wrote: > On Tue, 01 Feb 2022 05:49:05 PST (-0800), binutils@sourceware.org wrote: >> This commit relaxes requirements to `fmv.s' instructions from F to (F or >> Zfinx).  The same applies to `fmv.d' and `fmv.q'. >> >> gas/ChangeLog: >> >>     * testsuite/gas/riscv/zfinx.s: Add `fmv.s' instruction. >>     * testsuite/gas/riscv/zfinx.d: Likewise. >>     * testsuite/gas/riscv/zdinx.s: Add `fmv.d' instruction. >>     * testsuite/gas/riscv/zdinx.d: Likewise. >>     * testsuite/gas/riscv/zqinx.d: Add `fmv.q' instruction. >>     * testsuite/gas/riscv/zqinx.s: Likewise. >> >> opcodes/ChangeLog: >> >>     * riscv-opc.c (riscv_opcodes): Relax requirements to >>     `fmv.[sdq]' instructions to support those in Zfinx/Zdinx/Zqinx. >> --- >>  gas/testsuite/gas/riscv/zdinx.d | 1 + >>  gas/testsuite/gas/riscv/zdinx.s | 1 + >>  gas/testsuite/gas/riscv/zfinx.d | 1 + >>  gas/testsuite/gas/riscv/zfinx.s | 1 + >>  gas/testsuite/gas/riscv/zqinx.d | 1 + >>  gas/testsuite/gas/riscv/zqinx.s | 1 + >>  opcodes/riscv-opc.c             | 6 +++--- >>  7 files changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/gas/testsuite/gas/riscv/zdinx.d b/gas/testsuite/gas/riscv/zdinx.d >> index cb465bfbef4..3db2cb56f1a 100644 >> --- a/gas/testsuite/gas/riscv/zdinx.d >> +++ b/gas/testsuite/gas/riscv/zdinx.d >> @@ -36,6 +36,7 @@ Disassembly of section .text: >>  [     ]+[0-9a-f]+:[     ]+a2c58553[     ]+fle.d[     ]+a0,a1,a2 >>  [     ]+[0-9a-f]+:[     ]+a2b61553[     ]+flt.d[     ]+a0,a2,a1 >>  [     ]+[0-9a-f]+:[     ]+a2b60553[     ]+fle.d[     ]+a0,a2,a1 >> +[     ]+[0-9a-f]+:[     ]+22b58553[     ]+fmv.d[     ]+a0,a1 >>  [     ]+[0-9a-f]+:[     ]+22b59553[     ]+fneg.d[     ]+a0,a1 >>  [     ]+[0-9a-f]+:[     ]+22b5a553[     ]+fabs.d[     ]+a0,a1 >>  [     ]+[0-9a-f]+:[     ]+e2059553[     ]+fclass.d[     ]+a0,a1 >> diff --git a/gas/testsuite/gas/riscv/zdinx.s b/gas/testsuite/gas/riscv/zdinx.s >> index f44358111de..cdf5f3c2e7e 100644 >> --- a/gas/testsuite/gas/riscv/zdinx.s >> +++ b/gas/testsuite/gas/riscv/zdinx.s >> @@ -28,6 +28,7 @@ target: >>      fle.d    a0, a1, a2 >>      fgt.d    a0, a1, a2 >>      fge.d    a0, a1, a2 >> +    fmv.d    a0, a1 >>      fneg.d    a0, a1 >>      fabs.d    a0, a1 >>      fclass.d    a0, a1 >> diff --git a/gas/testsuite/gas/riscv/zfinx.d b/gas/testsuite/gas/riscv/zfinx.d >> index 6465c08ea9a..6fc4491fbc0 100644 >> --- a/gas/testsuite/gas/riscv/zfinx.d >> +++ b/gas/testsuite/gas/riscv/zfinx.d >> @@ -34,6 +34,7 @@ Disassembly of section .text: >>  [     ]+[0-9a-f]+:[     ]+a0c58553[     ]+fle.s[     ]+a0,a1,a2 >>  [     ]+[0-9a-f]+:[     ]+a0b61553[     ]+flt.s[     ]+a0,a2,a1 >>  [     ]+[0-9a-f]+:[     ]+a0b60553[     ]+fle.s[     ]+a0,a2,a1 >> +[     ]+[0-9a-f]+:[     ]+20b58553[     ]+fmv.s[     ]+a0,a1 >>  [     ]+[0-9a-f]+:[     ]+20b59553[     ]+fneg.s[     ]+a0,a1 >>  [     ]+[0-9a-f]+:[     ]+20b5a553[     ]+fabs.s[     ]+a0,a1 >>  [     ]+[0-9a-f]+:[     ]+e0059553[     ]+fclass.s[     ]+a0,a1 >> diff --git a/gas/testsuite/gas/riscv/zfinx.s b/gas/testsuite/gas/riscv/zfinx.s >> index 41ae0e38ad4..d63c0c37570 100644 >> --- a/gas/testsuite/gas/riscv/zfinx.s >> +++ b/gas/testsuite/gas/riscv/zfinx.s >> @@ -26,6 +26,7 @@ target: >>      fle.s    a0, a1, a2 >>      fgt.s    a0, a1, a2 >>      fge.s    a0, a1, a2 >> +    fmv.s    a0, a1 >>      fneg.s    a0, a1 >>      fabs.s    a0, a1 >>      fclass.s    a0, a1 >> diff --git a/gas/testsuite/gas/riscv/zqinx.d b/gas/testsuite/gas/riscv/zqinx.d >> index e8d2b7ba4c5..c704241bc90 100644 >> --- a/gas/testsuite/gas/riscv/zqinx.d >> +++ b/gas/testsuite/gas/riscv/zqinx.d >> @@ -38,6 +38,7 @@ Disassembly of section .text: >>  [     ]+[0-9a-f]+:[     ]+a6c58553[     ]+fle.q[     ]+a0,a1,a2 >>  [     ]+[0-9a-f]+:[     ]+a6b61553[     ]+flt.q[     ]+a0,a2,a1 >>  [     ]+[0-9a-f]+:[     ]+a6b60553[     ]+fle.q[     ]+a0,a2,a1 >> +[     ]+[0-9a-f]+:[     ]+26b58553[     ]+fmv.q[     ]+a0,a1 >>  [     ]+[0-9a-f]+:[     ]+26b59553[     ]+fneg.q[     ]+a0,a1 >>  [     ]+[0-9a-f]+:[     ]+26b5a553[     ]+fabs.q[     ]+a0,a1 >>  [     ]+[0-9a-f]+:[     ]+e6059553[     ]+fclass.q[     ]+a0,a1 >> diff --git a/gas/testsuite/gas/riscv/zqinx.s b/gas/testsuite/gas/riscv/zqinx.s >> index ecfa509b98c..02147b1919c 100644 >> --- a/gas/testsuite/gas/riscv/zqinx.s >> +++ b/gas/testsuite/gas/riscv/zqinx.s >> @@ -30,6 +30,7 @@ target: >>      fle.q    a0, a1, a2 >>      fgt.q    a0, a1, a2 >>      fge.q    a0, a1, a2 >> +    fmv.q    a0, a1 >>      fneg.q    a0, a1 >>      fabs.q    a0, a1 >>      fclass.q    a0, a1 > > Looking at the ISA manual, I'm not actually seeing Zqinx defined.  Specifically > >    \begin{commentary} >    In the future, an RV64Zqinx quad-precision extension could be defined analogously >    to RV32Zdinx. >    An RV32Zqinx extension could also be defined but would require >    quad-register groups. >    \end{commentary} > > Looks like it was removed from the ISA manual here > >    9025a7f Remove Zqinx (for now, at least) > > I must have missed that when reviewing the patches last time, but not entirely sure what to do as we're about to release it.  None of that shouldn't block this patch set, though. I'm aware of the status of Zqinx. Still, if Zqinx is going to be ratified, its encoding will be likely the same as the one implemented in GNU Binutils. So my idea for Zqinx is: "fix, then remove temporarily". If we remove Zqinx extension with a single commit, it will be a lot easier to restore once that extension is ratified. Removing Zqinx without destroying current implementation is easy. It's just simple diff as follows: diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c index 142cd1f0d1f..19649429d01 100644 --- a/bfd/elfxx-riscv.c +++ b/bfd/elfxx-riscv.c @@ -1186,7 +1186,6 @@ static struct riscv_supported_ext riscv_supported_std_z_ext[] = {"zihintpause", ISA_SPEC_CLASS_DRAFT, 1, 0, 0 }, {"zfinx", ISA_SPEC_CLASS_DRAFT, 1, 0, 0 }, {"zdinx", ISA_SPEC_CLASS_DRAFT, 1, 0, 0 }, - {"zqinx", ISA_SPEC_CLASS_DRAFT, 1, 0, 0 }, {"zbb", ISA_SPEC_CLASS_DRAFT, 1, 0, 0 }, {"zba", ISA_SPEC_CLASS_DRAFT, 1, 0, 0 }, {"zbc", ISA_SPEC_CLASS_DRAFT, 1, 0, 0 }, (also, we will also need to remove Zqinx-related testcases) I think we can *cleanly* remove Zqinx extension after we resolve Zfinx issues. I have no objection about removing Zqinx *in the process* resolving Zfinx issues, though. Thanks, Tsukasa > >> diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c >> index 2da0f7cf0a4..991d4d7a0aa 100644 >> --- a/opcodes/riscv-opc.c >> +++ b/opcodes/riscv-opc.c >> @@ -598,7 +598,7 @@ const struct riscv_opcode riscv_opcodes[] = >>  {"fmv.w.x",    0, INSN_CLASS_F,   "D,s",       MATCH_FMV_S_X, MASK_FMV_S_X, match_opcode, 0 }, >>  {"fmv.x.s",    0, INSN_CLASS_F,   "d,S",       MATCH_FMV_X_S, MASK_FMV_X_S, match_opcode, 0 }, >>  {"fmv.s.x",    0, INSN_CLASS_F,   "D,s",       MATCH_FMV_S_X, MASK_FMV_S_X, match_opcode, 0 }, >> -{"fmv.s",      0, INSN_CLASS_F,   "D,U",       MATCH_FSGNJ_S, MASK_FSGNJ_S, match_rs1_eq_rs2, INSN_ALIAS }, >> +{"fmv.s",      0, INSN_CLASS_F_OR_ZFINX,   "D,U",       MATCH_FSGNJ_S, MASK_FSGNJ_S, match_rs1_eq_rs2, INSN_ALIAS }, >>  {"fneg.s",     0, INSN_CLASS_F_OR_ZFINX,   "D,U",       MATCH_FSGNJN_S, MASK_FSGNJN_S, match_rs1_eq_rs2, INSN_ALIAS }, >>  {"fabs.s",     0, INSN_CLASS_F_OR_ZFINX,   "D,U",       MATCH_FSGNJX_S, MASK_FSGNJX_S, match_rs1_eq_rs2, INSN_ALIAS }, >>  {"fsgnj.s",    0, INSN_CLASS_F_OR_ZFINX,   "D,S,T",     MATCH_FSGNJ_S, MASK_FSGNJ_S, match_opcode, 0 }, >> @@ -656,7 +656,7 @@ const struct riscv_opcode riscv_opcodes[] = >>  {"fsd",        0, INSN_CLASS_D_AND_C, "CD,Cl(Cs)", MATCH_C_FSD, MASK_C_FSD, match_opcode, INSN_ALIAS|INSN_DREF|INSN_8_BYTE }, >>  {"fsd",        0, INSN_CLASS_D,   "T,q(s)",    MATCH_FSD, MASK_FSD, match_opcode, INSN_DREF|INSN_8_BYTE }, >>  {"fsd",        0, INSN_CLASS_D,   "T,A,s",     0, (int) M_FSD, match_never, INSN_MACRO }, >> -{"fmv.d",      0, INSN_CLASS_D,   "D,U",       MATCH_FSGNJ_D, MASK_FSGNJ_D, match_rs1_eq_rs2, INSN_ALIAS }, >> +{"fmv.d",      0, INSN_CLASS_D_OR_ZDINX,   "D,U",       MATCH_FSGNJ_D, MASK_FSGNJ_D, match_rs1_eq_rs2, INSN_ALIAS }, >>  {"fneg.d",     0, INSN_CLASS_D_OR_ZDINX,   "D,U",       MATCH_FSGNJN_D, MASK_FSGNJN_D, match_rs1_eq_rs2, INSN_ALIAS }, >>  {"fabs.d",     0, INSN_CLASS_D_OR_ZDINX,   "D,U",       MATCH_FSGNJX_D, MASK_FSGNJX_D, match_rs1_eq_rs2, INSN_ALIAS }, >>  {"fsgnj.d",    0, INSN_CLASS_D_OR_ZDINX,   "D,S,T",     MATCH_FSGNJ_D, MASK_FSGNJ_D, match_opcode, 0 }, >> @@ -713,7 +713,7 @@ const struct riscv_opcode riscv_opcodes[] = >>  {"flq",        0, INSN_CLASS_Q,   "D,A,s",     0, (int) M_FLQ, match_never, INSN_MACRO }, >>  {"fsq",        0, INSN_CLASS_Q,   "T,q(s)",    MATCH_FSQ, MASK_FSQ, match_opcode, INSN_DREF|INSN_16_BYTE }, >>  {"fsq",        0, INSN_CLASS_Q,   "T,A,s",     0, (int) M_FSQ, match_never, INSN_MACRO }, >> -{"fmv.q",      0, INSN_CLASS_Q,   "D,U",       MATCH_FSGNJ_Q, MASK_FSGNJ_Q, match_rs1_eq_rs2, INSN_ALIAS }, >> +{"fmv.q",      0, INSN_CLASS_Q_OR_ZQINX,   "D,U",       MATCH_FSGNJ_Q, MASK_FSGNJ_Q, match_rs1_eq_rs2, INSN_ALIAS }, >>  {"fneg.q",     0, INSN_CLASS_Q_OR_ZQINX,   "D,U",       MATCH_FSGNJN_Q, MASK_FSGNJN_Q, match_rs1_eq_rs2, INSN_ALIAS }, >>  {"fabs.q",     0, INSN_CLASS_Q_OR_ZQINX,   "D,U",       MATCH_FSGNJX_Q, MASK_FSGNJX_Q, match_rs1_eq_rs2, INSN_ALIAS }, >>  {"fsgnj.q",    0, INSN_CLASS_Q_OR_ZQINX,   "D,S,T",     MATCH_FSGNJ_Q, MASK_FSGNJ_Q, match_opcode, 0 }, > > Presumably whomever wrote this assumed these were flavors of move, but the ISA manual is pretty clear that they're not > >    Note, FSGNJ.S {\em rx, ry, ry} moves {\em ry} to {\em rx} (assembler    pseudoinstruction FMV.S {\em rx, ry}); > > Thus they're in Zfinx, as they're not explicitly omitted. > > Reviewed-by: Palmer Dabbelt > > Thanks! >