Bug/packld nanbox - #389
Open
RunjiaChen wants to merge 2 commits into
Open
Conversation
vx_packlb_f / vx_packlh_f write a 32-bit float into a 64-bit FLEN
register, so RISC-V requires the upper word to be NaN-boxed (all ones).
SimX does this; the RTL does not, and the existing packld test cannot
see the difference.
The blindness is structural, not a weak assertion. kernel.cpp stores each
result through a float*, which is a 32-bit fsw, and main.cpp compares
four bytes. The NaN-box lives in the upper half and is discarded before
it ever reaches memory, so both drivers pass.
packld_box is a copy of packld with the observation window widened to the
full 64-bit container:
- kernel.cpp stores uint64_t instead of float, and reads the packed
result with fmv.x.d fused into the same asm block as the packed load
(as separate statements the compiler may spill the float through a
32-bit fsw/flw pair, which would fabricate the very bits under test);
- main.cpp sizes the destination buffers for uint64_t and counts the
packed VALUE and the NaN-BOX as separate error classes, so a box-only
failure cannot be confused with a mis-packed value.
Measured, upstream tooling only, default config (1 cluster, 1 core,
4 warps, 4 threads; EXT_D on so FLEN=64 and boxing is required):
ci/blackbox.sh --driver=simx --app=packld_box -> PASSED
value errors = 0, nanbox errors = 0
ci/blackbox.sh --driver=rtlsim --app=packld_box -> FAILED
value errors = 0, nanbox errors = 512
value errors = 0 on both drivers: the packing itself (byte order, strided
addressing, both widths) is correct everywhere. The NaN-box is the sole
divergence, and it fails on every one of the 512 packed loads.
The upper half is not merely un-set to ones, it is never written at all,
so it retains prior register content: one observed sample read
0x8d2428b1 rather than zero, which makes the architectural result of a
packed load depend on whatever the destination register happened to hold.
Root cause is in the RTL, not the test. VX_decode.sv gives packed loads
op_type INST_LSU_LBU / INST_LSU_LHU, VX_uop_packld.sv preserves that
op_type while rewriting only offset and bytesel, and VX_lsu_slice.sv
applies NaN-boxing solely on the LSU_FMT_W arm. The BU/HU arms are plain
zero-extends, so rsp_is_float is computed but never consulted for these
instructions. VX_uop_packld.sv's comment claiming the boxing "is handled
automatically ... triggers nan-box in lsu_slice" does not hold.
This change is purely additive: no existing file is modified. Adding
packld_box to the tests/regression/Makefile list is deliberately omitted
so nothing upstream changes; blackbox.sh --app=packld_box works without
it.
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.
Packed loads (
vx_packlb_f/vx_packlh_f) do not NaN-box their resultThis PR adds a reproducer test only — it does not fix the RTL. It makes an
existing defect observable and reproducible; the fix is left to whoever owns
the LSU, since (see Root cause below) it is not a one-line change.
Summary
vx_packlb_f/vx_packlh_fwrite a 32-bit float into a 64-bit FP register.With
EXT_Denabled (FLEN=64, the default config) RISC-V requires the upper32 bits to be NaN-boxed (all ones). SimX does this; the RTL does not.
The upper half is not merely left un-set to ones — for
packlbit is neverwritten, so it retains whatever the destination register previously held. The
architectural result of a packed load therefore depends on prior register
state.
Reproduction
Measured, default config (1 cluster / 1 core / 4 warps / 4 threads,
VX_CFG_EXT_D_ENABLED=1,NUM_THREADS=4 SIMD_WIDTH=4 NUM_ALU_LANES=4):PASSED!FAILED!512 = 16 tasks × 16 points × 2 instructions, i.e. every packed load.
value errors = 0on both drivers is the important half: the packing itself —byte order, strided addressing, both widths — is correct everywhere. The
NaN-box is the sole divergence.
Sample failures:
Reproduced on two independent machines (RHEL9 native, and Ubuntu 24.04 under
Docker/Rosetta). Counts are identical; the stale upper word differs between
machines (
0x8d2428b1vs0x01e5a123), which is itself the evidence thatthose bytes are never written rather than zeroed.
Why the existing
packldtest does not catch thistests/regression/packldis blind by construction, not by a weak assertion:The result is stored through a
float*(a 32-bitfsw), so the NaN-box isdiscarded before it ever reaches memory, and the host then compares four bytes.
The observation window is 32 bits wide and the defect is in the other 32 bits.
Both drivers report no failures.
What this PR contains
A single new directory,
tests/regression/packld_box/, derived fromtests/regression/packldwith the observation window widened:kernel.cpp: storesuint64_tinstead offloat, and reads the packedresult with
fmv.x.dfused into the same asm block as the packed load.As separate statements the compiler may spill the float through an
fsw/flwpair, which would fabricate the very bits under test. Verified:the compiled kernel contains 0
fsw/flwand 32fmv.x.d, one perpacked load.
main.cpp: buffers sized foruint64_t; the verify step counts the packedvalue and the NaN-box as separate error classes, so a box-only
failure cannot be mistaken for a mis-packed value.
Makefile,common.h: unchangedNo existing file is modified.
packld_boxis deliberately not added tothe
tests/regression/Makefileapp list, so the suite and CI are unaffected bya test that is expected to fail. Happy to wire it in as a regression guard once
a fix lands — that is arguably where it belongs.
Environment
Upstream
master@d76b7f24. Also confirmed present ate6fcb7d2, so this isneither new nor master-specific — and the diff between them does not touch
VX_uop_packld.sv(byte-identical) nor the boxing logic inVX_lsu_slice.sv.