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

[Jan Beulich]

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