[PATCH v5 2/2] RISC-V: Add SpacemiT vendor extension xsmtvdotii

[Jiawei]

在 2026/5/14 21:53, Mark Zhuang 写道:
> On Wed, 13 May 2026, Jiawei wrote:
>> First, in the disassembler handling of Xpx:
>>
>> if (wi == 2)
>> print (..., ",i4");
>> else if (wi != 3)
>> print (..., ",i8");
>>
>> Should the last case instead go to undefined_modifier? Xpx seems to allow
>> only WI == 2 and WI == 3, so WI == 0/1 should not be printed as i8.
> Agreed, will fix in the next version.
>
>> Second, for the .sp forms, the comment says that the Xpb/Xpw form is listed
>> first so that the disassembler picks it for round-trip correctness, but the
>> actual opcode table lists the Xpn/Xpx form first.
> The comment was wrong.  After analysis, the ordering does not affect
> observable behavior — Xpn and Xpb produce identical output, and Xpw/Xpx
> both suppress output for WI==3.  I will remove the misleading comment in
> the next version.
>
>> Also, Xpn is the stride 0..1 form in the assembler, while Xpb accepts 0..3.
>> The disassembler currently prints the raw 2-bit stride for both Xpn and Xpb.
>> Should the Xpn disassembler path reject stride values 2 and 3, so that
>> invalid i4 + stride 2/3 encodings are not printed as valid mnemonics?
> Your suggestion is correct in principle.  I tried to implement it but ran
> into a structural limitation.  I would like to discuss the right fix before
> sending an updated patch.
>
> The problem
> -----------
>
> `print_insn_args` is `void`.  When an operand handler calls
> `goto undefined_modifier`, it prints an error message and returns — but
> the outer opcode-matching loop has no way to detect the failure and cannot
> fall through to the next opcode table entry.
>
> For the .sp instructions there are two entries per mnemonic:
>
>    entry 1: ..., XpnXpx   (stride 0..1, i4 or i8)
>    entry 2: ..., XpbXpw   (stride 0..3, i8 only)
>
> With a stride check in `case 'n'`, a valid encoding stride=2/3 + WI=3
> (i8) hits `undefined_modifier` in entry 1 and the loop stops there,
> producing:
>
>    smt.vmadotu.sp  v2,v4,v5,v0,# internal error, undefined modifier (N)
>
> instead of the correct:
>
>    smt.vmadotu.sp  v2,v4,v5,v0,2
>
> Swapping the table order does not help: it fixes stride=2/3 + WI=3 but
> breaks stride=0/1 + WI=2 (i4) for the same reason.  The root cause is
> the missing fallback, not the table order.
>
> Proposed fix
> ------------
>
> Change `print_insn_args` to return `bool`, have `undefined_modifier`
> return `false` silently, and add a dry-run pass at the call site before
> committing to an opcode entry.  This touches the signature of
> `print_insn_args` and the behavior of `undefined_modifier`, both shared
> by all RISC-V instructions, not just this vendor extension.
>
> Does this approach look correct to you, or is there a cleaner existing
> mechanism in the disassembler infrastructure that I may have missed?
> And would you prefer it bundled with the xsmtvdot/xsmtvdotii patch, or
> sent separately?  If it can be handled independently, I am happy to
> prepare a follow-up patch after the vendor extension is merged.  If you
> think it should be resolved together, I will include it in the next
> version.
>
> Thanks,
> Mark

Thanks for the detailed analysis.

The Xpx fix sounds good to me.

For the Xpn/Xpb issue, your explanation makes sense. If print_insn_args()
cannot report failure back to the opcode matching loop, then using
undefined_modifier in the operand printer is not enough to let the
disassembler try the next matching opcode entry.

Changing print_insn_args() to return a status and adding a dry-run/fallback
step sounds like the right general direction to me, but it looks like a
generic RISC-V disassembler infrastructure change rather than something
specific to xsmtvdotii. I would prefer to keep that as a separate follow-up
patch, so it can be reviewed independently and checked against existing
RISC-V disassembler behavior.

For the vendor extension patch, I think fixing the Xpx WI==0/1 case and
removing the misleading comment should be enough for the next version. The
remaining Xpn/Xpb fallback limitation can be handled by the follow-up
disassembler cleanup.

Of course, I would defer to the maintainers on whether the infrastructure
change should be a prerequisite for this extension.

Thanks,
Jiawei