Skip to content

fix(riscv): reject vslide1up/vfslide1up with vd == vs2 - #199

Closed
carlosqwqqwq wants to merge 2 commits into
HexRaysSA:masterfrom
carlosqwqqwq:fix-riscv-vslide1up-vd-vs2
Closed

fix(riscv): reject vslide1up/vfslide1up with vd == vs2#199
carlosqwqqwq wants to merge 2 commits into
HexRaysSA:masterfrom
carlosqwqqwq:fix-riscv-vslide1up-vd-vs2

Conversation

@carlosqwqqwq

@carlosqwqqwq carlosqwqqwq commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

fix(riscv): reject vslide1up/vfslide1up with vd == vs2

Closes #194

Summary

The RISC-V V extension reserves vslide1up/vfslide1up when the destination aliases the vs2 source register, while vslide1down/vfslide1down permit the overlap. vd == vs2 now raises an illegal-instruction trap for the up forms.

Validation

  • Regression uses the correct encodings (funct3=0b110 for the .vx forms and 0b101 for the .vf forms), covering vslide1up.vx, vfslide1up.vf, the legal vslide1down.vx/vfslide1down.vf controls, and a non-overlapping legal form.
  • cargo test --lib: cpu suite 72 passed, 0 failed.

Notes

19h commented Aug 11, 2026

Copy link
Copy Markdown
Member

Blocking review finding: the added regression test encodes the wrong instructions and currently fails.

  • 0x3a1140d7 has funct3=0b100, so RAX decodes it as vslideup.vx, not vslide1up.vx. The correct vslide1up.vx v1,v1,x2 word is 0x3a1160d7 (funct3=0b110).
  • Likewise, 0x3e1140d7 is vslidedown.vx; the vslide1down.vx control is 0x3e1160d7.
  • The floating forms use funct3=0b101: 0x3a1150d7 / 0x3e1150d7.

The focused command ran one test and failed at the first assertion:

vslide1up_rejects_vd_eq_vs2 ... FAILED
0 passed; 1 failed

Issue #194 contains the same incorrect probe. Its current word actually exposes a separate pre-existing vslideup overlap bug, because that handler also lacks the required group-overlap validation. Please correct the issue/test, add the vfslide1up case, and either fix or separately track the vslideup defect. 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.
vslide1up and vfslide1up write vd while reading vs2, so aliasing the source register is a reserved encoding; vslide1down and vfslide1down permit the overlap. Regression covers both trap forms and both legal down controls.
@carlosqwqqwq

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Fixed as suggested:

  • The regression now uses the correct encodings: vslide1up.vx v1,v1,x2 = 0x3a1160d7 (funct3=0b110), vslide1down.vx v1,v1,x2 = 0x3e1160d7, and the FP forms vfslide1up.vf/vfslide1down.vf = 0x3a1150d7/0x3e1150d7 (funct3=0b101).
  • vfslide1up is now covered, and the vslide1down/vfslide1down overlap controls confirm they stay legal.
  • The vslideup group-overlap gap (exposed by the old mis-decoded word) is tracked separately in issue riscv: vslideup accepts partial vd/vs2 group overlap #204; this PR stays scoped to vslide1up/vfslide1up.
  • 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.

vslide1up/vfslide1up are accepted with vd == vs2

2 participants