fix .vf scalar: NaN-box a narrow scalar FP operand - #88
Merged
Conversation
Contributor
|
Thank you! |
Contributor
Author
|
Note: this check needs a delivery-side fix in rocket-chip to work. On stock rocket-chip the |
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.
Hello, here is a pull request for a bug I found.
When a .vf scalar floating-point operand is read narrower than the FP register
(vs1_eew < 64) and its upper bits are not all ones, RISC-V requires it to read as
the canonical NaN of that width. Saturn passed the raw low bits, so a wider value
held in the register but consumed by a narrower .vf op (for example a double used
by a single-precision .vf) is treated as a valid float and produces a wrong
result.
The fix NaN-boxes the scalar at its source in ExecuteSequencer (0x7FC00000 for
e32, 0x7E00 for e16 when the upper bits are not all ones). Backend already splats
io.iss.bits.scalar into the per-lane operand the FP units read, so boxing this one
field covers every consumer. Non-.vf paths are unchanged.
Found by differential testing against Spike; the reduced reproducer
(vfwnmacc.vf with an unboxed scalar, seed 3217) diverges before and matches Spike
after.
Note: the defect was confirmed against Spike with the reproducer on the Saturn
version my fuzzer runs, and this is the same fix adapted to current master, where the
operand is still passed unboxed. I have not run a full Chisel build of the adapted
version (Saturn builds only through Chipyard), so a run through the cosim tests
before merging would be good.