Re: [PATCH v2 2/2] RISC-V: Better support for long instructions

[Tsukasa OI <research_trasio@irq.a4lg.com>]

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 <research_trasio@irq.a4lg.com>
>>>>
>>>> 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