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

[Jan Beulich <jbeulich@suse.com>]

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.

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