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

[Tsukasa OI]

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.

> 
> 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