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 5C28B384F6DF for ; Thu, 24 Nov 2022 07:35:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5C28B384F6DF 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 3E67D300089; Thu, 24 Nov 2022 07:35:37 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=irq.a4lg.com; s=2017s01; t=1669275337; bh=ZqK6J0vwAwUMHJtN3yIeiaRQg6zjs7qY5dg9iK8nVTI=; h=Message-ID:Date:Mime-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type:Content-Transfer-Encoding; b=X5jRgYT26XN8oTAQ19qOxaWFTZ4DMufuFgUrsIaWqEsloyFnhp+nffVh12RU4vhRS nqWqaXrK7g2Z8lM98fvfYjnRvAPUf1xBRIFDICJs8ton+T4bpEeZ+avNHkZD74Y2Wp 6FdUjJsRkXNuhPKGaLA6TgvuhLObGnk8v9w/O09E= Message-ID: <367bb34b-bd8e-5537-03b3-c12501e80708@irq.a4lg.com> Date: Thu, 24 Nov 2022 16:35:35 +0900 Mime-Version: 1.0 Subject: Re: [PATCH v2 2/2] RISC-V: Better support for long instructions Content-Language: en-US To: Jan Beulich Cc: binutils@sourceware.org References: <0263444d-3077-8de9-6b40-5643fa60e705@irq.a4lg.com> <826cc496-e176-5ad7-cd74-8670a612892a@suse.com> From: Tsukasa OI In-Reply-To: <826cc496-e176-5ad7-cd74-8670a612892a@suse.com> 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/24 16:31, Jan Beulich wrote: > On 24.11.2022 03:34, Tsukasa OI wrote: >> 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. > > But the assembler was rejecting the input there originally, wasn't it? > At which point _extending_ the testcase is certainly wanted, but do you > really need to check the ".byte ..." output to achieve the goal of the > test? > >>> 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. > > I agree three would be too much; I wonder whether > > 1. Disassembler fix > 2. Assembler fix + Test > > wouldn't be the better way to split, if you are going to in the first > place. Aiui the disassembler fix doesn't need any adjustments to > testcases, whereas my view is that the testcase addition is primarily > about the previously wrongly rejected assembler input, and only > secondarily about disassembler output. Hence keeping the testcase > adjustments with the assembler fix would, to me, seem more "natural". > > Jan > Ah, I can agree that as well and I feel your option more natural. I somehow missed that (probably because of my health issues for a weeks). Anyway, thanks for pointing this out! Tsukasa