Skip to content

[AMDGPU] comgr: fix SR FP8 carry-propagation in E5M3 stochastic rounding#3275

Open
suryajasper wants to merge 4 commits into
amd-stagingfrom
users/sujasper/hotswap-sr-fp8-carry-fix
Open

[AMDGPU] comgr: fix SR FP8 carry-propagation in E5M3 stochastic rounding#3275
suryajasper wants to merge 4 commits into
amd-stagingfrom
users/sujasper/hotswap-sr-fp8-carry-fix

Conversation

@suryajasper

@suryajasper suryajasper commented Jul 8, 2026

Copy link
Copy Markdown
Member

Summary

Fix carry-propagation bug in patchCvtSrFp8F32. The old stochastic noise injection path extracted the F32 mantissa, added noise, and masked back to 23 bits -- discarding the carry when mantissa + noise overflowed. This caused 3.1% of stochastic rounding results to be off by exactly 7 (the full UE5M3 mantissa range).

Root cause

// OLD (buggy): extract, add, mask loses carry
v_and_b32 tmp, 0x007FFFFF, out    // extract mantissa
v_lshrrev_b32 out, 12, src1       // shift noise
v_add_u32 tmp, tmp, out           // add noise to mantissa
v_and_b32 tmp, 0x007FFFFF, tmp    // mask -- DISCARDS CARRY
v_max_num_f32 out, 0, src0        // reload source
v_bfi_b32 out, 0x007FFFFF, tmp, out  // reinsert mantissa

Fix

// NEW (correct): add noise directly to F32 bitpattern
v_lshrrev_b32 tmp, 12, src1       // shift noise
v_add_u32 out, out, tmp           // carry propagates naturally

Integer addition on the whole F32 bitpattern lets mantissa overflow carry into the exponent, matching the ISA's stochastic rounding behavior. Also reduces trampoline from 6 to 2 instructions.

Covered instructions

  • v_cvt_sr_fp8_f32 with CLAMP=1 (UE5M3 stochastic rounding)

Changes

File Change
comgr-hotswap-patch-f32-to-e5m3.cpp Replace 6-instruction noise injection with 2-instruction direct add
hotswap-cvt-sr-fp8.s Update CHECK patterns for shorter noise injection sequence
hotswap-cvt-fp8-modifiers.s Update CHECK patterns for SR with neg/abs source modifiers

Test plan

  • Validated on gfx1250 A0 hardware via internal test suite (covers stochastic rounding with multiple seeds, edge cases including negative values, NaN, inf, boundary max, byte_sel variants, and source modifier combinations)
  • All hotswap lit tests pass (55/55 on MI300)
  • PSDB CI green

The stochastic noise injection in patchCvtSrFp8F32 extracted the F32
mantissa, added noise, then masked with & 0x007FFFFF. When mantissa +
noise overflowed 23 bits, the carry into the exponent field was
discarded, causing the UE5M3 byte to wrap within the same exponent
instead of advancing to the next one.

Fix: add the shifted noise directly to the F32 bitpattern. Integer
addition naturally carries from mantissa into exponent, matching ISA
behavior. This also reduces the trampoline from 6 to 2 instructions.

Affected instruction: v_cvt_sr_fp8_f32 with CLAMP=1 and non-zero seed.
Symptom: 3.1% of lanes produce output 7 less than correct (mantissa
zeroed instead of exponent incremented) when the truncated UE5M3 byte
has mantissa=0b111 and SR decides to round up.
… fix

Remove stale v_and_b32 0x7fffff (mantissa mask) CHECK patterns from
SR_NEG and SR_ABS check blocks in hotswap-cvt-fp8-modifiers.s. The
carry-propagation fix changed the SR noise injection path from a
6-instruction extract/add/mask sequence to a 2-instruction direct
F32 bitpattern addition (v_lshrrev_b32 + v_add_u32), which no longer
includes the mantissa mask step.
@suryajasper suryajasper force-pushed the users/sujasper/hotswap-sr-fp8-carry-fix branch from 38e3701 to d20a206 Compare July 9, 2026 00:11
Comment thread amd/comgr/src/comgr-hotswap-patch-f32-to-e5m3.cpp Outdated
@chinmaydd chinmaydd added comgr Related to Code Object Manager hotswap Related to the Comgr Hotswap feature labels Jul 9, 2026
@chinmaydd

Copy link
Copy Markdown

Where is the test cvt_sr_fp8_nonzero_seed that is mentioned in the description?

@chinmaydd

chinmaydd commented Jul 9, 2026

Copy link
Copy Markdown

How are we really ensuring numeric correctness here ? Can you add tests that exercise the corner cases ? negative values, NaNs, Infs, etc.

Shorten the noise injection comment to describe current behavior
without referencing the old buggy code path.
@suryajasper

suryajasper commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

Where is the test cvt_sr_fp8_nonzero_seed that is mentioned in the description?

These are part of our internal testing repo, where we added unit test kernels that directly test these corner cases. This is where we originally identified these failures, and I validated they all pass with this patch added. I've removed them from the description since we don't have those here within llvm.

How are we really ensuring numeric correctness here ? Can you add tests that exercise the corner cases ? negative values, NaNs, Infs, etc.

We can't validate numerical correctness through lit tests. There's also no A0 unit testing CI in llvm-project that I can hook into for hardware validation. However, all these instruction conversion patterns are covered by the lit tests in this PR.

Numerical correctness was validated on gfx1250 A0 hardware using our internal test suite, which exercises v_cvt_sr_fp8_f32 CLAMP=1 across these corner cases:

  • Negative values (clamped to 0 by UE5M3 unsigned format)
  • NaN passthrough (0xFF)
  • +inf → 0xFE (UE5M3 max)
  • Boundary max values (65504, 65536, 114688)
  • All byte_sel variants (0, 1, 2, 3)
  • Stochastic rounding with multiple seeds (seed=0 and seed=nonzero)
  • Source modifier combinations (neg, abs)
  • Pack → unpack roundtrips

All 19 tests pass with 0/256 mismatches against CPU reference implementations.

@suryajasper suryajasper requested a review from lamb-j July 9, 2026 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comgr Related to Code Object Manager hotswap Related to the Comgr Hotswap feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants