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 95A613858D35 for ; Tue, 29 Nov 2022 03:19:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 95A613858D35 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 F2421300089; Tue, 29 Nov 2022 03:19:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=irq.a4lg.com; s=2017s01; t=1669691969; bh=mRSFf0FqLvtJJJhx7OEhE8BGr01viip+CUKoiZ1v2CY=; h=Message-ID:Date:Mime-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type:Content-Transfer-Encoding; b=UkS4BhkLh6K5jGkw1WJAqLDCIwd+OH02toIw0cGnY5cJOXoG7CI4qmHuLMXPLXWnU WSIUXrAgGz8NqJFAwYLYGN7IdJUGSsWjFDbHcd/yxao13FXXOm2JMx/W60IYACcG+v WMsyit5Xz81knEg26wPEYhKaP/QCIKmX6nK0sb6k= Message-ID: <5dd6e382-8f91-1d75-5307-b3a18db52e1c@irq.a4lg.com> Date: Tue, 29 Nov 2022 12:19:27 +0900 Mime-Version: 1.0 Subject: Re: [PATCH 1/3] RISC-V: Allocate "various" operand type Content-Language: en-US To: Nelson Chu Cc: binutils@sourceware.org References: From: Tsukasa OI In-Reply-To: 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 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: Nelson, Before the real reply... - The reply is going to be pretty long. - I'm not going to reply to you today to stop myself from griping (not to be led by my frustrations) Meanwhile, reviewing (at least, PATCH 1-7/11 should be easy to review and partial approval of PATCH 1-7/11 is okay to me) would be nice. Sincerely, Tsukasa On 2022/11/29 11:42, Nelson Chu wrote: > On Mon, Nov 28, 2022 at 2:39 PM Tsukasa OI wrote: >> >> From: Tsukasa OI >> >> This commit intends to move operands that require very special handling or >> operand types that are so minor (e.g. only useful on a few instructions) >> under "W". I also intend this "W" to be "temporary" operand storage until >> we can find good two character (or less) operand type. >> >> In this commit, prefetch offset operand "f" for 'Zicbop' extension is moved >> to "Wif" because of its special handling (and allocating single character >> "f" for this operand type seemed too much). >> >> Current expected allocation guideline is as follows: >> >> 1. 'W' >> 2. The most closely related single-letter extension in lowercase >> (strongly recommended but not mandatory) >> 3. Identify operand type >> >> The author currently plans to allocate following three-character operand >> types (for operands including instructions from unratified extensions). >> >> 1. "Wif" ('Zicbop': fetch offset) > > Maybe just "if" without the W? W seems redundant. If the offset is > immediate then using "i + " seems more clear to understand. > >> 2. "Wfv" (unratified 'Zfa': value operand from FLI.[HSDQ] instructions) > > We probably also need to modify other old operand names to comply with > the new naming rules. But for now the current operand names seem > enough, maybe we can find better naming rules in the future, it's not > urgent. > >> 3. "Wfm" / "WfM" >> 'Zfh', 'F', 'D', 'Q': rounding modes "m" with special handling >> solely for widening conversion instructions. > > I still think these similar checks, including the rounding mode > checks, should be checked in different match_opcode functions, that > should be enough. There is no way to make assembly wrong until you > are using the .insn directives, and we don't need to check if the > operands are valid or not for them, since .insn is used to allow users > to write an unsupported instruction. Please don't continue trying to > add various operand names to support these, it is really hard to > maintain. I should have already said that, so this is the last time I > mention it. > > Nelson > >> gas/ChangeLog: >> >> * config/tc-riscv.c (validate_riscv_insn, riscv_ip): Move from >> "f" to "Wif". >> >> opcodes/ChangeLog: >> >> * riscv-dis.c (print_insn_args): Move from "f" to "Wif". >> * riscv-opc.c (riscv_opcodes): Reflect new operand type. >> --- >> gas/config/tc-riscv.c | 64 +++++++++++++++++++++++++++++++------------ >> opcodes/riscv-dis.c | 26 ++++++++++++++---- >> opcodes/riscv-opc.c | 6 ++-- >> 3 files changed, 71 insertions(+), 25 deletions(-) >> >> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c >> index 0682eb355241..bb0e18ac8d52 100644 >> --- a/gas/config/tc-riscv.c >> +++ b/gas/config/tc-riscv.c >> @@ -1359,7 +1359,6 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length) >> case 'j': used_bits |= ENCODE_ITYPE_IMM (-1U); break; >> case 'a': used_bits |= ENCODE_JTYPE_IMM (-1U); break; >> case 'p': used_bits |= ENCODE_BTYPE_IMM (-1U); break; >> - case 'f': /* Fall through. */ >> case 'q': used_bits |= ENCODE_STYPE_IMM (-1U); break; >> case 'u': used_bits |= ENCODE_UTYPE_IMM (-1U); break; >> case 'z': break; /* Zero immediate. */ >> @@ -1386,6 +1385,21 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length) >> goto unknown_validate_operand; >> } >> break; >> + case 'W': /* Various operands. */ >> + switch (*++oparg) >> + { >> + case 'i': >> + switch (*++oparg) >> + { >> + case 'f': used_bits |= ENCODE_STYPE_IMM (-1U); break; >> + default: >> + goto unknown_validate_operand; >> + } >> + break; >> + default: >> + goto unknown_validate_operand; >> + } >> + break; >> case 'X': /* Integer immediate. */ >> { >> size_t n; >> @@ -3401,22 +3415,37 @@ riscv_ip (char *str, struct riscv_cl_insn *ip, expressionS *imm_expr, >> imm_expr->X_op = O_absent; >> continue; >> >> - case 'f': /* Prefetch offset, pseudo S-type but lower 5-bits zero. */ >> - if (riscv_handle_implicit_zero_offset (imm_expr, asarg)) >> - continue; >> - my_getExpression (imm_expr, asarg); >> - check_absolute_expr (ip, imm_expr, false); >> - if (((unsigned) (imm_expr->X_add_number) & 0x1fU) >> - || imm_expr->X_add_number >= (signed) RISCV_IMM_REACH / 2 >> - || imm_expr->X_add_number < -(signed) RISCV_IMM_REACH / 2) >> - as_bad (_("improper prefetch offset (%ld)"), >> - (long) imm_expr->X_add_number); >> - ip->insn_opcode |= >> - ENCODE_STYPE_IMM ((unsigned) (imm_expr->X_add_number) & >> - ~ 0x1fU); >> - imm_expr->X_op = O_absent; >> - asarg = expr_end; >> - continue; >> + case 'W': /* Various operands. */ >> + switch (*++oparg) >> + { >> + case 'i': >> + switch (*++oparg) >> + { >> + case 'f': >> + /* Prefetch offset for 'Zicbop' extension. >> + pseudo S-type but lower 5-bits zero. */ >> + if (riscv_handle_implicit_zero_offset (imm_expr, asarg)) >> + continue; >> + my_getExpression (imm_expr, asarg); >> + check_absolute_expr (ip, imm_expr, false); >> + if (((unsigned) (imm_expr->X_add_number) & 0x1fU) >> + || imm_expr->X_add_number >= RISCV_IMM_REACH / 2 >> + || imm_expr->X_add_number < -RISCV_IMM_REACH / 2) >> + as_bad (_ ("improper prefetch offset (%ld)"), >> + (long) imm_expr->X_add_number); >> + ip->insn_opcode |= ENCODE_STYPE_IMM ( >> + (unsigned) (imm_expr->X_add_number) & ~0x1fU); >> + imm_expr->X_op = O_absent; >> + asarg = expr_end; >> + continue; >> + default: >> + goto unknown_riscv_ip_operand; >> + } >> + break; >> + default: >> + goto unknown_riscv_ip_operand; >> + } >> + break; >> >> case 'X': /* Integer immediate. */ >> { >> @@ -3469,6 +3498,7 @@ riscv_ip (char *str, struct riscv_cl_insn *ip, expressionS *imm_expr, >> } >> } >> break; >> + >> default: >> unknown_riscv_ip_operand: >> as_fatal (_("internal: unknown argument type `%s'"), >> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c >> index 0e1f3b4610aa..1e6716e8e58c 100644 >> --- a/opcodes/riscv-dis.c >> +++ b/opcodes/riscv-dis.c >> @@ -473,11 +473,6 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info >> (int)EXTRACT_STYPE_IMM (l)); >> break; >> >> - case 'f': >> - print (info->stream, dis_style_address_offset, "%d", >> - (int)EXTRACT_STYPE_IMM (l)); >> - break; >> - >> case 'a': >> info->target = EXTRACT_JTYPE_IMM (l) + pc; >> (*info->print_address_func) (info->target, info); >> @@ -582,6 +577,27 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info >> print (info->stream, dis_style_immediate, "%d", rs1); >> break; >> >> + case 'W': /* Various operands. */ >> + { >> + switch (*++oparg) >> + { >> + case 'i': >> + switch (*++oparg) >> + { >> + case 'f': >> + print (info->stream, dis_style_address_offset, "%d", >> + (int) EXTRACT_STYPE_IMM (l)); >> + break; >> + default: >> + goto undefined_modifier; >> + } >> + break; >> + default: >> + goto undefined_modifier; >> + } >> + } >> + break; >> + >> case 'X': /* Integer immediate. */ >> { >> size_t n; >> diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c >> index 0e691544f9bc..653eb60f2a58 100644 >> --- a/opcodes/riscv-opc.c >> +++ b/opcodes/riscv-opc.c >> @@ -313,9 +313,9 @@ const struct riscv_opcode riscv_opcodes[] = >> /* name, xlen, isa, operands, match, mask, match_func, pinfo. */ >> >> /* Standard hints. */ >> -{"prefetch.i", 0, INSN_CLASS_ZICBOP, "f(s)", MATCH_PREFETCH_I, MASK_PREFETCH_I, match_opcode, 0 }, >> -{"prefetch.r", 0, INSN_CLASS_ZICBOP, "f(s)", MATCH_PREFETCH_R, MASK_PREFETCH_R, match_opcode, 0 }, >> -{"prefetch.w", 0, INSN_CLASS_ZICBOP, "f(s)", MATCH_PREFETCH_W, MASK_PREFETCH_W, match_opcode, 0 }, >> +{"prefetch.i", 0, INSN_CLASS_ZICBOP, "Wif(s)", MATCH_PREFETCH_I, MASK_PREFETCH_I, match_opcode, 0 }, >> +{"prefetch.r", 0, INSN_CLASS_ZICBOP, "Wif(s)", MATCH_PREFETCH_R, MASK_PREFETCH_R, match_opcode, 0 }, >> +{"prefetch.w", 0, INSN_CLASS_ZICBOP, "Wif(s)", MATCH_PREFETCH_W, MASK_PREFETCH_W, match_opcode, 0 }, >> {"pause", 0, INSN_CLASS_ZIHINTPAUSE, "", MATCH_PAUSE, MASK_PAUSE, match_opcode, 0 }, >> >> /* Basic RVI instructions and aliases. */ >> -- >> 2.38.1 >> >