[https://nvbugs/6581067][fix] Request the unbiased GEMM in fp32 and add bias.float() before a single cast… - #17505
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe FP4 bias parity test now computes its reference in float32 before casting to bfloat16. Four CUBLASLt and CUTLASS skip waivers were removed. The tolerance rationale now covers autotuner and accumulation-order differences. ChangesFP4 bias validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change aligns the fp32 reference computation with the fused bf16 behavior and removes obsolete test waivers; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description clearly explains the double-rounding root cause, the fix, the removed waivers, and the relevant test command. It includes test coverage evidence, although it does not use the template headings exactly and omits the PR checklist confirmation. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
Confirmed fp32 out_dtype is accepted by all three backends this test parametrizes (fp4Gemm.cpp:198, cublasFp4ScaledMM.cpp:75, cudaNvfp4MM.cpp), so the unwaived cuda_core case at mnk0 isn't affected. The fix makes the reference strictly more accurate, which tightens the assertion rather than loosening it.
One correction for the description: at |out|≈374 the effective budget is atol + rtol*|ref| ≈ 3.7, so the failure was rtol-dominated (2 ULP = 4.0), not "exceeding atol=5e-3". Worth fixing so the next reader doesn't chase the wrong term.
239b497 to
583795d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
yiqingy0
left a comment
yiqingy0
left a comment
There was a problem hiding this comment.
Approved for infra part. Need to trigger CI.
|
/bot run --disable-fail-fast |
|
PR_Github #69413 [ run ] triggered by Bot. Commit: |
|
PR_Github #69413 [ run ] completed with state
|
…p4_gemm_bias_per_backend The reference out_no_bias + bias rounded to bf16 twice while the fused epilogue rounds once, so at |out|~374 (1 bf16 ULP = 2.0) cancellation against the bias promoted the remainder the reference had already discarded into a multi-ULP error, exceeding atol=5e-3. Compute the unbiased GEMM in fp32 and add the bias in fp32, rounding to bf16 exactly once. The fused result is then bit-identical to the reference across 4 shapes x 2 backends x 5 seeds, so the original tolerance is retained rather than widened. Remove the now-passing waivers for this bug. Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
583795d to
0f09f5c
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/bot run --disable-fail-fast |
|
PR_Github #69666 [ run ] triggered by Bot. Commit: |
|
PR_Github #69666 [ run ] completed with state |
8 participants
Summary
nvfp4_gemm(bfloat16) + biasrounds to bf16 twice whereas the fused epilogue rounds once, so at |out|≈374 (1 bf16 ULP = 2.0) cancellation against the bias promotes the reference's already-discarded remainder into a multi-ULP error exceeding atol=5e-3.bias.float()before a single cast to bf16, making the reference single-rounded so the original tight tolerance passes unchanged (fused becomes bit-identical across 4 shapes × 2 backends × 5 seeds); removed the 4 now-passing waivers.pytest tests/unittest/_torch/thop/parallel/test_fp4_linear.py::test_fp4_gemm_bias_per_backend -k "mnk2 or mnk3" -vTest plan
Links
Dev Engineer Review
float32.bias.float()before one cast tobfloat16.QA Engineer Review
tests/unittest/_torch/thop/parallel/test_fp4_linear.py.tests/integration/test_lists/waives.txt.