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 612653858D1E for ; Thu, 24 Nov 2022 02:34:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 612653858D1E 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 EF1CF300089; Thu, 24 Nov 2022 02:34:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=irq.a4lg.com; s=2017s01; t=1669257263; bh=1NGNN5PmmegDaOwBoVR6pNWrFGRNbcBqNNNZJtZEawY=; h=Message-ID:Date:Mime-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type:Content-Transfer-Encoding; b=ENEXubZVCkK1neQkolKoK27Q9Tvq8t5z/c1bgFTrXhXB6n+K0nGl66M0jZFS9Ez1q rV2cIF8lh0Gojt6vtZf4ZtibzMEIH0TGYBFCI/Igjz0YExuJjcUsixNeiy/3/V4uoV YBr+uwlkIKPVRSFh8L8xtxlLVcZxGWmlj4YCL9bs= Message-ID: <0263444d-3077-8de9-6b40-5643fa60e705@irq.a4lg.com> Date: Thu, 24 Nov 2022 11:34:19 +0900 Mime-Version: 1.0 Subject: Re: [PATCH v2 2/2] RISC-V: Better support for long instructions To: Jan Beulich Cc: binutils@sourceware.org, Nelson Chu References: Content-Language: en-US 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/11/23 18:04, Jan Beulich wrote: > On 23.11.2022 09:30, Tsukasa OI wrote: >> From: Tsukasa OI >> >> Commit bb996692bd96 ("RISC-V/gas: allow generating up to 176-bit >> instructions with .insn") tried to start supporting long instructions but >> it was insufficient. >> >> 1. It heavily depended on the bignum internals (radix of 2^16), >> 2. It generates "value conflicts with instruction length" even if a big >> number instruction encoding does not exceed its expected length, >> 3. Because long opcode was handled separately (from struct riscv_cl_insn), >> some information like DWARF line number correspondence was missing and >> 4. On the disassembler, disassembler dump was limited up to 64-bit. >> For long (unknown) instructions, instruction bits are incorrectly >> zeroed out. >> >> To solve these problems, this commit: >> >> 1. Handles bignum (and its encodings) precisely, >> 2. Incorporates long opcode handling into regular >> struct riscv_cl_insn-handling functions and >> 3. Adds packet argument to support dumping instructions >> longer than 64-bits. >> >> gas/ChangeLog: >> >> * config/tc-riscv.c (struct riscv_cl_insn): Add long opcode field. >> (create_insn) Clear long opcode marker. >> (install_insn) Install longer opcode as well. >> (s_riscv_insn) Likewise. >> (riscv_ip_hardcode): Make big number handling stricter. Length and >> the value conflicts only if the bignum size exceeds the expected >> maximum length. >> * testsuite/gas/riscv/insn.s: Add testcases such that big number >> handling is required. >> * testsuite/gas/riscv/insn.d: Likewise. >> * testsuite/gas/riscv/insn-na.d: Likewise. >> * testsuite/gas/riscv/insn-dwarf.d: Likewise. >> >> opcodes/ChangeLog: >> >> * riscv-dis.c (riscv_disassemble_insn): Print unknown instruction >> using the new argument packet. >> (riscv_disassemble_data): Add unused argument packet. >> (print_insn_riscv): Pass packet to the disassemble function. > > The code changes look okay to me. For the testsuite additions I have > voiced my reservations, and I've given further background in an earlier > reply still on the v1 sub-thread. Whatever the resolution there would > imo want to be applied here as well. Understood. My (minimum) opinion is, I want to keep 22-bytes patterns corresponding PATCH v2 2/2 because that's exactly changed by the assembler / disassembler fixes. > > As to mixing assembler and disassembler changes in the same patch: Is > this strictly necessary here for some reason? Generally I would suggest > to split such, but once again I wouldn't insist on you doing so ... > > Jan > I'm okay to split: - Assembler fix + Disassembler fix + Test to: 1. Assembler fix 2. Disassembler fix + Test but there are a good reason I did like this in this patch. To test fixed assembler, we need to fix disassembler as well. Although they are not exactly the same issue, they are corresponding enough so that merging changes might be justified. But since they are not the same issue (as you pointed out), I'm okay to split to two (three might be too much) separate patches. Thanks, Tsukasa