Skip to content

fix(riscv): reject partial vd/vs2 overlap in narrowing ops - #200

Closed
carlosqwqqwq wants to merge 2 commits into
HexRaysSA:masterfrom
carlosqwqqwq:fix-riscv-narrowing-partial-overlap
Closed

fix(riscv): reject partial vd/vs2 overlap in narrowing ops#200
carlosqwqqwq wants to merge 2 commits into
HexRaysSA:masterfrom
carlosqwqqwq:fix-riscv-narrowing-partial-overlap

Conversation

@carlosqwqqwq

@carlosqwqqwq carlosqwqqwq commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

fix(riscv): reject partial vd/vs2 overlap in narrowing ops

Closes #195

Summary

RVV §5.2 permits a destination group to overlap a narrowing source group only in its lowest-numbered part (vd == vs2); misaligned groups and partial overlap are reserved encodings. The validation computes the source group size from the rational EMUL (2*LMUL), so fractional LMUL configurations keep a one-register source group and odd vs2 registers remain legal.

Validation

  • CPU regression covers fractional LMUL (e32,mf2 fully-overlapped and odd disjoint vs2), m1/m2 alignment rules, legal lowest-part overlap, disjoint groups, and illegal partial/misaligned overlaps.
  • cargo test --lib: cpu suite 72 passed, 0 failed.

Notes

The vector semantics moved to cpu/vector.rs in #202 so this change does not grow the oversized cpu.rs.

19h commented Aug 11, 2026

Copy link
Copy Markdown
Member

Blocking review finding: the new register-count calculation rejects legal fractional-LMUL narrowing encodings.

The code first rounds every fractional LMUL to one occupied destination register and then computes source_regs = group_regs * 2. For LMUL=1/2:

architectural source EMUL = 2 × 1/2 = 1 register
proposed source_regs      = 2 × ceil(1/2) = 2 registers

The subsequent vs2 % source_regs check therefore rejects every odd vs2, even though any register is a valid one-register source group. For example, under e32,mf2, fully overlapped vnsrl.wv v1, v1, v2 is legal but this patch traps because 1 % 2 != 0. The same problem affects LMUL=1/4 and 1/8.

Please calculate EMUL as a rational value before converting it to occupied registers, and add regression cases covering fractional LMUL, odd/even register numbers, legal lowest-part overlap, disjoint groups, and illegal partial overlap. The PR currently adds no executable regression tests. Spec: https://docs.riscv.org/reference/isa/unpriv/v-st-ext

Repository policy note: this PR adds to src/isa/riscv/cpu.rs, which is currently 7,221 lines / 264,801 bytes and exceeds both hard split triggers in AGENTS.md. The touched vector semantic group should be split here or in a prerequisite shared refactor.

cpu.rs exceeded the AGENTS.md hard split triggers (2000 lines / 150 kB). Move the vector (RVV) element access and data-path execution group, including set_vtype, into cpu/vector.rs with no behavior change.
Narrowing instructions read a 2*LMUL vs2 group and write an LMUL vd; only the lowest-part overlap vd == vs2 is legal. Reject misaligned groups and partial overlap. With fractional LMUL the source EMUL is below one register, so the source group occupies a single register and odd vs2 registers remain legal.
@carlosqwqqwq

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Reworked as suggested:

  • The register-count calculation now derives the source group from the rational EMUL: fractional LMUL (mf2/mf4/mf8) keeps a one-register source group, so e32,mf2 vnsrl.wv v1,v1,v2 and odd vs2 registers are legal again. m1/m2/m4 use 2/4/8-register source groups with the corresponding alignment; m8 (2*LMUL > 8) is rejected.
  • Added an executable regression: fractional LMUL fully-overlapped and odd disjoint vs2, m1/m2 alignment, legal lowest-part overlap, disjoint groups, and illegal partial/misaligned overlaps.
  • Repository policy: the touched vector semantic group is split into src/isa/riscv/cpu/vector.rs in the prerequisite refactor refactor(riscv): split vector semantics out of cpu.rs #202.

Verified locally: cargo test --lib --no-default-features --features smir-jit cpu suite 72 passed, 0 failed.

@19h

19h commented Aug 11, 2026

Copy link
Copy Markdown
Member

Thanks @carlosqwqqwq. This fix is being consolidated into rollup PR #203, where it shares centralized direct-interpreter, SMIR/JIT, and QEMU differential coverage with the related RVV corrections. Closing this PR in favor of #203; the relevant rollup commits credit you as co-author.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

narrowing instructions are executed with partial vd/vs2 group overlap

2 participants