Repository navigation
[rocjitsu] Correct legacy floating min/max and median selection - #13898
Conversation
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
kuhar
changed the title
There was a problem hiding this comment.
The changes to the Legacy logic seem fine. However, the results do not match HW. RDNA 1/2 show the same two deviations already fixed for RDNA 3_5/4.
The manual is just incorrect for two cases: NaN propogration, and zero ordering -0 < +0
Can you verify the claims against CDNA HW too? The changes can be easily propagated if the behavior repeats.
Click for more details (Agent's Description)
Ran a directed probe against RDNA1 and RDNA2 silicon to check the new legacy selection path: an RX 5700 XT (gfx1010) and an RX 6800 XT (gfx1030), every pair and triple of 14 encodings (±0, ±denormal, ±1, ±inf, ±qNaN with distinct payloads, ±sNaN) across 16 opcodes × 2,744 lanes × 32 MODEs (IEEE 0/1 × both FP_DENORM fields) = 1,404,928 results per GPU. The rerun is byte-identical and the two GPUs are byte-identical to each other.
Most of the model holds up exactly: one NaN → the numeric source, MODE.IEEE sNaN priority (a before b, quieted), IEEE=0 leaving the encoding untouched, input flushing before selection, and MED3-any-NaN → MIN3 all reproduce bit for bit.
The per-target table is the problem. LegacyPolicy{prefer_first_nan = true, ordered_zero_median = true} reproduces all 1,404,928 results on both parts; the {false, false} arm these targets currently sit in misses 122,880. Witnesses at MODE 0x0f0:
v_max_f32 7fc00000, 7fc12345 -> 7fc00000 (src0, not src1)
v_max_f64 7ff8000000000000, 7ff8000012345678 -> 7ff8000000000000
v_med3_f32 -0, +0, -0 -> -0 (-0 below +0)
v_med3_f32 7fc00000, 7fc12345, ffc00000 -> 7fc00000 (src0, not src2)
Two knock-ons beyond the inline suggestions: legacy_minmax_test.cpp currently asserts the contradicted behavior for these params, so the suite passes today and would need updating alongside; and I have no CDNA part available, so the CDNA1–4 entries are untouched by this and remain unverified in both fields.
0fcd5de to
3a1b154
Compare
sjain-stanford
left a comment
sjain-stanford
left a comment
There was a problem hiding this comment.
I think it would help to close the remaining selection-test gaps while adding this coverage (paths below are under emulation/rocjitsu/):
- In
tests/simd_correctness/vop2_minmax_simd_correctness_test.cpp:214, remove the NaN/signed-zero lane exclusion and its obsolete comments. It hides the repaired cases in mixed-lane and partial-EXEC checks. Both tests pass with the exclusion disabled. - In
tests/legacy_minmax_test.cpp:241, drive the F32 and F16/F64 input-denormal fields differently, and include F16 VOP2 alongside VOP3. Setting both fields to the same value cannot detect a wrong-field read; the existing F16 VOP2 cases contain no subnormals. - Add CDNA1/2 to the parameter lists at
tests/legacy_minmax_test.cpp:409and:589: their regenerated executors use these opcodes too. The existing cases pass for those targets (40 passed, four expected skips). - Add signaling-NaN cases to
PackedNumberSelectionattests/legacy_minmax_test.cpp:562. The branch atlib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/shared/fp_mode.h:413correctly selects number semantics today, but the current denormal-only test cannot distinguish that from RDNA legacy selection. Two sNaNs with IEEE=0 and an sNaN/number pair with IEEE=1 exercise the difference; the analogous dual-issue cases already exist.
3a1b154 to
26867bb
Compare
Host min/max loses the NaN priority and signed-zero ordering required by older CDNA and RDNA instructions. Median also selects the wrong operand when a source is NaN, and unpacked selection misses MODE input-denormal flushing. Extend the shared encoding selector with legacy NaN policies and MODE.IEEE signaling-NaN handling. Use it for scalar, SIMD, dual-issue and packed F16 execution. Apply input flushing before selection and preserve selected denormals. Preserve SDWA output scaling after raw F16 selection. Use MIN3 for median inputs containing NaNs. Match the measured RDNA1-3 first-NaN priority and median signed-zero ordering, and retain the ISA rules for CDNA. Require an explicit target policy for legacy selection, reject unsupported targets without throwing, and regenerate the affected executors. Cover denormal modes, signed zeros, NaN source and payload priority, packed clamp, CDNA1-4 F16/F32/F64, RDNA1/2 scalar and SIMD selection, RDNA vector and dual-issue forms, and RDNA3.5 scalar F16/F32 operations. Check independent denormal fields, SDWA scaling and packed number semantics, and compare every lane in scalar/SIMD equivalence tests. Keep CI independent of the uninitialized tensor-DMA fixtures added upstream by temporarily excluding only the 11 affected cases.
26867bb to
8542f32
Compare
github-actions
Bot
added
the
github actions
Host min/max loses the NaN priority and signed-zero ordering required by
older CDNA and RDNA instructions. Median also selects the wrong operand
when a source is NaN, and unpacked selection misses MODE input-denormal
flushing.
Extend the shared encoding selector with legacy NaN policies and
MODE.IEEE signaling-NaN handling. Use it for scalar, SIMD, dual-issue
and packed F16 execution. Apply input flushing before selection and
preserve selected denormals. Use MIN3 for median inputs containing NaNs.
Match the measured RDNA3 median ordering of signed zeros while retaining
the documented rule for older targets. Require an explicit target policy
for legacy selection and regenerate the affected executors.
Cover denormal modes, signed zeros, NaN source and payload priority,
packed clamp, CDNA3/4 F16/F32/F64, RDNA vector and dual-issue forms, and
RDNA3.5 scalar F16/F32 operations.