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 8B4A83858C5E for ; Mon, 31 Jul 2023 08:01:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8B4A83858C5E 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 563EF300089; Mon, 31 Jul 2023 08:01:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=irq.a4lg.com; s=2017s01; t=1690790474; bh=CBSLeUHSNu9eW/vAjgtO2dnhtf6d1jqjdTaPtp52VBQ=; h=Message-ID:Date:Mime-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type:Content-Transfer-Encoding; b=WrK9dA3VpsAeF4iaqCe1r1HYzboFzPXCOXn4Vs7gmiF27tfzsaJaUGEKCNC8ERip+ Dwt8y17viXyNbzELudFAlE9xxjRQU15nvk0pd1gGv1pfUYTzH9Zsfq8+E3p6vUaxfg KEfA6WLPYXc3VK1FsZd8z1QeR57pxze6cHBv1by0= Message-ID: Date: Mon, 31 Jul 2023 17:01:13 +0900 Mime-Version: 1.0 Subject: Re: [PATCH 2/2] RISC-V: Support Zcmp cm.mv instructions Content-Language: en-US To: Jiawei Cc: binutils@sourceware.org References: <20230726090622.630672-1-jiawei@iscas.ac.cn> <20230726090622.630672-2-jiawei@iscas.ac.cn> From: Tsukasa OI In-Reply-To: <20230726090622.630672-2-jiawei@iscas.ac.cn> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.1 required=5.0 tests=BAYES_00,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.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: OK, minor and major reviews follow. On 2023/07/26 18:06, Jiawei wrote: > This patch supports Zcmp instruction 'cm.mva01s' and 'cm.mvsa01'. > All disassemble instructions use the sreg format. > > Co-Authored by: Charlie Keaney > Co-Authored by: Mary Bennett > Co-Authored by: Nandni Jamnadas > Co-Authored by: Sinan Lin > Co-Authored by: Simon Cook > Co-Authored by: Shihua Liao > Co-Authored by: Yulong Shi > > gas/ChangeLog: > > * config/tc-riscv.c (validate_riscv_insn): New operators. > (riscv_ip): Ditto. > * testsuite/gas/riscv/zcmp-mv.d: New test. > * testsuite/gas/riscv/zcmp-mv.s: New test. > > include/ChangeLog: > > * opcode/riscv-opc.h (MATCH_CM_MVA01S): New opcode. > (MASK_CM_MVA01S): New mask. > (MATCH_CM_MVSA01): New opcode. > (MASK_CM_MVSA01): New mask. > (DECLARE_INSN): New declarations. > * opcode/riscv.h (OP_MASK_SREG1): New mask. > (OP_SH_SREG1): New operand code. > (OP_MASK_SREG2): New mask. > (OP_SH_SREG2): New operand code. > (X_A0): New reg number. > (X_A1): Ditto. > (X_S7): Ditto. > (RISCV_SREG_0_7): New macro function. > > opcodes/ChangeLog: > > * riscv-dis.c (riscv_get_sregno): > (print_insn_args): > * riscv-opc.c (match_sreg1_not_eq_sreg2): > > --- > gas/config/tc-riscv.c | 17 +++++++++++++++++ > gas/testsuite/gas/riscv/zcmp-mv.d | 26 ++++++++++++++++++++++++++ > gas/testsuite/gas/riscv/zcmp-mv.s | 21 +++++++++++++++++++++ > include/opcode/riscv-opc.h | 6 ++++++ > include/opcode/riscv.h | 12 ++++++++++++ > opcodes/riscv-dis.c | 19 +++++++++++++++++++ > opcodes/riscv-opc.c | 8 ++++++++ > 7 files changed, 109 insertions(+) > create mode 100644 gas/testsuite/gas/riscv/zcmp-mv.d > create mode 100644 gas/testsuite/gas/riscv/zcmp-mv.s > > diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c > index 266a91451b7..c5be308aa08 100644 > --- a/gas/config/tc-riscv.c > +++ b/gas/config/tc-riscv.c > @@ -1578,6 +1578,9 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length) > case 'c': > switch (*++oparg) > { > + /* sreg operators in cm.mvsa01 and cm.mva01s. */ > + case '1': USE_BITS (OP_MASK_SREG1, OP_SH_SREG1); break; > + case '2': USE_BITS (OP_MASK_SREG2, OP_SH_SREG2); break; > /* byte immediate operators, load/store byte insns. */ > case 'h': used_bits |= ENCODE_ZCB_HALFWORD_UIMM (-1U); break; > /* halfword immediate operators, load/store halfword insns. */ > @@ -3785,6 +3788,20 @@ riscv_ip (char *str, struct riscv_cl_insn *ip, expressionS *imm_expr, > asarg = expr_parse_end; > imm_expr->X_op = O_absent; > continue; > + > + case '1': Indentation: 2 tabs and 4 spaces before "case". > + if (!reg_lookup (&asarg, RCLASS_GPR, ®no) > + || !RISCV_SREG_0_7 (regno)) > + break; > + INSERT_OPERAND (SREG1, *ip, regno % 8); IMO, plain (regno % 8) needs to be wrapped. I understand what you are doing but since this is not self-explanatory as x8-x15 (s0-s7 == x8-x9,x18-x23), I would prefer making a macro, working as a reverse of riscv_get_sregno in riscv-dis.c. /* I will put this kind of macro in include/opcode/riscv.h. The name is weird but the word "ENCODE" is used with different meanings in riscv.h. */ #define CONVERT_REGNO_TO_SREG(regno) (regno % 8) > + continue; > + > + case '2': Indentation: 2 tabs and 4 spaces before "case". > + if (!reg_lookup (&asarg, RCLASS_GPR, ®no) > + || !RISCV_SREG_0_7 (regno)) > + break; > + INSERT_OPERAND (SREG2, *ip, regno % 8); See above. > + continue; > > default: > goto unknown_riscv_ip_operand; > diff --git a/gas/testsuite/gas/riscv/zcmp-mv.d b/gas/testsuite/gas/riscv/zcmp-mv.d I'll prefer merging zcmp-mv and zcmp-push-pop. > new file mode 100644 > index 00000000000..351d301dd3f > --- /dev/null > +++ b/gas/testsuite/gas/riscv/zcmp-mv.d > @@ -0,0 +1,26 @@ > +#as: -march=rv64i_zcmp > +#source: zcmp-mv.s > +#objdump: -dr -Mno-aliases > + > +.*:[ ]+file format .* > + > + > +Disassembly of section .text: > + > +0+000 : > +[ ]*[0-9a-f]+:[ ]+ac7e[ ]+cm.mva01s[ ]+s0,s7 > +[ ]*[0-9a-f]+:[ ]+ac7a[ ]+cm.mva01s[ ]+s0,s6 > +[ ]*[0-9a-f]+:[ ]+acfe[ ]+cm.mva01s[ ]+s1,s7 > +[ ]*[0-9a-f]+:[ ]+acfa[ ]+cm.mva01s[ ]+s1,s6 > +[ ]*[0-9a-f]+:[ ]+afee[ ]+cm.mva01s[ ]+s7,s3 > +[ ]*[0-9a-f]+:[ ]+ade2[ ]+cm.mva01s[ ]+s3,s0 > +[ ]*[0-9a-f]+:[ ]+aef2[ ]+cm.mva01s[ ]+s5,s4 > +[ ]*[0-9a-f]+:[ ]+aefa[ ]+cm.mva01s[ ]+s5,s6 > +[ ]*[0-9a-f]+:[ ]+afa2[ ]+cm.mvsa01[ ]+s7,s0 > +[ ]*[0-9a-f]+:[ ]+af22[ ]+cm.mvsa01[ ]+s6,s0 > +[ ]*[0-9a-f]+:[ ]+afa6[ ]+cm.mvsa01[ ]+s7,s1 > +[ ]*[0-9a-f]+:[ ]+af26[ ]+cm.mvsa01[ ]+s6,s1 > +[ ]*[0-9a-f]+:[ ]+adbe[ ]+cm.mvsa01[ ]+s3,s7 > +[ ]*[0-9a-f]+:[ ]+ada2[ ]+cm.mvsa01[ ]+s3,s0 > +[ ]*[0-9a-f]+:[ ]+aeb2[ ]+cm.mvsa01[ ]+s5,s4 > +[ ]*[0-9a-f]+:[ ]+aeba[ ]+cm.mvsa01[ ]+s5,s6 > diff --git a/gas/testsuite/gas/riscv/zcmp-mv.s b/gas/testsuite/gas/riscv/zcmp-mv.s > new file mode 100644 > index 00000000000..0bcf2a6cd98 > --- /dev/null > +++ b/gas/testsuite/gas/riscv/zcmp-mv.s > @@ -0,0 +1,21 @@ > +target: > + > + # cm.mva01s > + cm.mva01s s0,s7 > + cm.mva01s s0,s6 > + cm.mva01s s1,s7 > + cm.mva01s s1,s6 > + cm.mva01s s7,s3 > + cm.mva01s x19,s0 > + cm.mva01s s5,x20 > + cm.mva01s x21,x22 > + > + # cm.mvsa01 > + cm.mvsa01 s7,s0 > + cm.mvsa01 s6,s0 > + cm.mvsa01 s7,s1 > + cm.mvsa01 s6,s1 > + cm.mvsa01 s3,s7 > + cm.mvsa01 x19,s0 > + cm.mvsa01 s5,x20 > + cm.mvsa01 x21,x22 > diff --git a/include/opcode/riscv-opc.h b/include/opcode/riscv-opc.h > index f8054f54f15..11e9c0b99d0 100644 > --- a/include/opcode/riscv-opc.h > +++ b/include/opcode/riscv-opc.h > @@ -2244,6 +2244,10 @@ > #define MASK_CM_POPRET 0xff03 > #define MATCH_CM_POPRETZ 0xbc02 > #define MASK_CM_POPRETZ 0xff03 > +#define MATCH_CM_MVA01S 0xac62 > +#define MASK_CM_MVA01S 0xfc63 > +#define MATCH_CM_MVSA01 0xac22 > +#define MASK_CM_MVSA01 0xfc63 > /* Svinval instruction. */ > #define MATCH_SINVAL_VMA 0x16000073 > #define MASK_SINVAL_VMA 0xfe007fff > @@ -3419,6 +3423,8 @@ DECLARE_INSN(cm_push, MATCH_CM_PUSH, MASK_CM_PUSH) > DECLARE_INSN(cm_pop, MATCH_CM_POP, MASK_CM_POP) > DECLARE_INSN(cm_popret, MATCH_CM_POPRET, MASK_CM_POPRET) > DECLARE_INSN(cm_popretz, MATCH_CM_POPRETZ, MASK_CM_POPRETZ) > +DECLARE_INSN(cm_mvsa01, MATCH_CM_MVSA01, MASK_CM_MVSA01) > +DECLARE_INSN(cm_mva01s, MATCH_CM_MVA01S, MASK_CM_MVA01S) > /* Vendor-specific (T-Head) XTheadBa instructions. */ > DECLARE_INSN(th_addsl, MATCH_TH_ADDSL, MASK_TH_ADDSL) > /* Vendor-specific (T-Head) XTheadBb instructions. */ > diff --git a/include/opcode/riscv.h b/include/opcode/riscv.h > index f9bfb19bd5e..44576fa9241 100644 > --- a/include/opcode/riscv.h > +++ b/include/opcode/riscv.h > @@ -326,6 +326,10 @@ static inline unsigned int riscv_insn_length (insn_t insn) > /* Zc fields. */ > #define OP_MASK_RLIST 0xf > #define OP_SH_RLIST 4 > +#define OP_MASK_SREG1 0x7 > +#define OP_SH_SREG1 7 > +#define OP_MASK_SREG2 0x7 > +#define OP_SH_SREG2 2 > > #define NVECR 32 > #define NVECM 1 > @@ -341,7 +345,10 @@ static inline unsigned int riscv_insn_length (insn_t insn) > #define X_T2 7 > #define X_S0 8 > #define X_S1 9 > +#define X_A0 10 > +#define X_A1 11 X_A0 and X_A1 are not used. Remove unless you are going to add 'Zcmp' instructions to the RISC-V simulator. > #define X_S2 18 > +#define X_S7 23 > #define X_S10 26 > #define X_S11 27 > #define X_T3 28 > @@ -389,6 +396,11 @@ static inline unsigned int riscv_insn_length (insn_t insn) > /* The maximal number of subset can be required. */ > #define MAX_SUBSET_NUM 4 > > +/* The range of sregs. */ > +#define RISCV_SREG_0_7(REGNO) \ > + ((REGNO == X_S0 || REGNO == X_S1) \ > + || (REGNO >= X_S2 && REGNO <= X_S7)) There's two points. 1. I think capitalized "REGNO" is not a good idea. "regno" will work and will look less like a constant. 2. How about the name "VALIDATE_SREG_REGNO" (just like "VALIDATE_U_IMM"?) > + > /* All RISC-V instructions belong to at least one of these classes. */ > enum riscv_insn_class > { > diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c > index d938c9cb2ed..058698c0c0a 100644 > --- a/opcodes/riscv-dis.c > +++ b/opcodes/riscv-dis.c > @@ -263,6 +263,17 @@ riscv_get_spimm (insn_t l) > return spimm; > } > > +/* Get s-register regno by using sreg number. > + e.g. the regno of s0 is 8, so > + riscv_get_sregno (0) equals 8. */ Indentation: 3 spaces before "e.g." and "riscv_get_sregno". About the function name "riscv_get_sregno", I'll comment later. > + > +static unsigned > +riscv_get_sregno (unsigned sreg_idx) Is this that generic (as I asked in riscv_get_spimm)? Also (on this function), it sounds like this function returns "sregno", not actual register number. Even you don't want 'Zcmp'-related prefix, I would suggest a function name like "riscv_get_regno_from_sregno". ... thinking of that, I started to think that the same idea may apply to "riscv_get_spimm" in PATCH 1/2. > +{ > + return sreg_idx > 1 ? > + sreg_idx + 16 : sreg_idx + 8; > +} > + > /* Print insn arguments for 32/64-bit code. */ > > static void > @@ -667,6 +678,14 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info > case 'c': /* Zcb extension 16 bits length instruction fields. */ > switch (*++oparg) > { > + case '1': > + print (info->stream, dis_style_register, "%s", > + riscv_gpr_names[riscv_get_sregno (EXTRACT_OPERAND (SREG1, l))]); > + break; > + case '2': > + print (info->stream, dis_style_register, "%s", > + riscv_gpr_names[riscv_get_sregno (EXTRACT_OPERAND (SREG2, l))]); > + break; > case 'b': > print (info->stream, dis_style_immediate, "%d", > (int)EXTRACT_ZCB_BYTE_UIMM (l)); > diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c > index 7d2f92e736b..b0a69efed49 100644 > --- a/opcodes/riscv-opc.c > +++ b/opcodes/riscv-opc.c > @@ -329,6 +329,12 @@ match_th_load_pair(const struct riscv_opcode *op, > return rd1 != rd2 && rd1 != rs && rd2 != rs && match_opcode (op, insn); > } > > +match_sreg1_not_eq_sreg2 (const struct riscv_opcode *op, insn_t insn) Before this line, put "static int" just like other opcode-matching functions. This is the most important of my comments in this e-mail. > +{ > + return match_opcode (op, insn) > + && (EXTRACT_OPERAND (SREG1, insn) != EXTRACT_OPERAND (SREG2, insn)); > +} > + > const struct riscv_opcode riscv_opcodes[] = > { > /* name, xlen, isa, operands, match, mask, match_func, pinfo. */ > @@ -1972,6 +1978,8 @@ const struct riscv_opcode riscv_opcodes[] = > {"cm.pop", 0, INSN_CLASS_ZCMP, "{Wcr},Wcp", MATCH_CM_POP, MASK_CM_POP, match_opcode, 0 }, > {"cm.popret", 0, INSN_CLASS_ZCMP, "{Wcr},Wcp", MATCH_CM_POPRET, MASK_CM_POPRET, match_opcode, 0 }, > {"cm.popretz", 0, INSN_CLASS_ZCMP, "{Wcr},Wcp", MATCH_CM_POPRETZ, MASK_CM_POPRETZ, match_opcode, 0 }, > +{"cm.mva01s", 0, INSN_CLASS_ZCMP, "Wc1,Wc2", MATCH_CM_MVA01S, MASK_CM_MVA01S, match_opcode, 0 }, > +{"cm.mvsa01", 0, INSN_CLASS_ZCMP, "Wc1,Wc2", MATCH_CM_MVSA01, MASK_CM_MVSA01, match_sreg1_not_eq_sreg2, 0 }, > > /* Supervisor instructions. */ > {"csrr", 0, INSN_CLASS_ZICSR, "d,E", MATCH_CSRRS, MASK_CSRRS|MASK_RS1, match_opcode, INSN_ALIAS }, In general, it's very good and some concerns about register numbers are avoided by design. Thanks, Tsukasa