Conversation
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).
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. |
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes #101.
In
vproc_mul_block, the generic operand and product registers are written every cycle. The control side invproc_mulonly moves stage N forward onstate_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 fromvproc_mulwith 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.
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.