Skip to content

Fix multiplier data stages advancing without valid (fixes #101) - #134

Closed
06lsm wants to merge 1 commit into
vproc:mainfrom
06lsm:fix-mul-stage-enable
Closed

06lsm wants to merge 1 commit into
vproc:mainfrom
06lsm:fix-mul-stage-enable

Conversation

@06lsm

@06lsm 06lsm commented Sep 23, 2026

Copy link
Copy Markdown

This fixes #101.

In vproc_mul_block, the generic operand and product registers are written every cycle. The control side in vproc_mul only moves stage N forward on state_exN_ready & state_ex(N-1)_valid_q. So when the pipeline stalls (commit delay or writeback backpressure), the data can run one beat ahead of its control, and a later result ends up with the product from another beat. That matches what was reported in #101 (wrong result equal to the previous one, only with commit delay).

The fix adds two enables to vproc_mul_block (ops_en_i, mul_en_i) and drives them from vproc_mul with the same conditions the control pipeline uses. No change to the arithmetic.

How I tested it: a randomized self-checking testbench (Verilator 4.228) around the real pipeline wrapper, i.e. unpacker, multiplier and the commit-gated packer. It runs vmul, vmulh, vmulhsu, vmacc and vsmul (RNU) with SEW 8/16/32 and LMUL 1/2/4/8, and randomly adds commit delays, writeback stalls and gaps between instructions. Results are compared against a separate C++ model of the RVV spec.

seeds instructions wrong bytes
current main 1-4 1,200 27,043
with this PR 1-204 61,200 0

The existing test suite passes as before (610/613; the three VREG_W=2048 kernel tests stop at elaboration with 4 WIDTHCONCAT warnings on main too).

Limits of the test: full VL, unmasked, RNU only, and it doesn't cover XIF decoding or vendor DSP multipliers. I can share the testbench if you want it.

@06lsm

06lsm commented Sep 23, 2026

Copy link
Copy Markdown
Author

A small follow-up on the numbers in the description: the original table only listed 4 seeds for the unpatched code. I've since run the unpatched code on the same 204 seeds as the patched version (same testbench, same build flow).

seeds with mismatches mismatching bytes / checked bytes
vicuna, original 204 / 204 1,403,323 / 3,672,000
vicuna, patched 0 / 204 0 / 3,672,000
vicuna2_core (OLD_VICUNA), original 204 / 204 56,206 / 3,672,000
vicuna2_core (OLD_VICUNA), patched 0 / 204 0 / 3,672,000

Same scope as before (VLEN=128, SEW 8/16/32, LMUL 1/2/4/8, unmasked, full VL). Happy to share the testbench if that helps with review.

@06lsm

06lsm commented Sep 24, 2026

Copy link
Copy Markdown
Author

Closing this as a duplicate of #130 by @Joao-Pedro-Cabral, which I missed before opening this PR, sorry. #130 is also more complete: it changes res_d to use acc_sub_q, and this PR does not. With add- and subtract-accumulate instructions back to back (e.g. vmacc.vv followed by vnmsac.vx), this PR still gives wrong results, and #130 passes the same randomized regression.

@06lsm 06lsm closed this Sep 24, 2026
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.

Wrong result generated by multiply unit (probably control logic related)

1 participant