[https://nvbugs/6450338][fix] Default to FP32 accumulation in allreduce fusion kernels#16753
[https://nvbugs/6450338][fix] Default to FP32 accumulation in allreduce fusion kernels#16753dongfengy wants to merge 1 commit into
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughFP32 accumulation in the all-reduce fusion kernel is now enabled by default. Setting ChangesFP32 accumulation control
Estimated code review effort: 1 (Trivial) | ~3 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/tensorrt_llm/kernels/communicationKernels/allReduceFusionKernels.cu (1)
720-727: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConst-qualify the cached environment pointer.
fp32_accis never reassigned and its contents are only inspected. Usechar const* constto enforce const-correctness and east-const style.As per coding guidelines: “Prefer
constorconstexprover macros, declare unmodified variablesconst, and use east-const style.”Proposed fix
- static char* fp32_acc = std::getenv("ALL_REDUCE_FUSION_KERNEL_ACC_FP32"); + static char const* const fp32_acc = std::getenv("ALL_REDUCE_FUSION_KERNEL_ACC_FP32");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tensorrt_llm/kernels/communicationKernels/allReduceFusionKernels.cu` around lines 720 - 727, Update the cached environment pointer in use_fp32_acc() to char const* const, preserving the existing getenv call and return logic.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cpp/tensorrt_llm/kernels/communicationKernels/allReduceFusionKernels.cu`:
- Around line 720-727: Update the cached environment pointer in use_fp32_acc()
to char const* const, preserving the existing getenv call and return logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1335b1b2-c5ef-42ee-b41e-5fadeb00b6aa
📒 Files selected for processing (1)
cpp/tensorrt_llm/kernels/communicationKernels/allReduceFusionKernels.cu
|
/bot run |
|
PR_Github #61078 [ run ] triggered by Bot. Commit: |
|
PR_Github #61078 [ run ] completed with state
|
|
/bot run |
1 similar comment
|
/bot run |
|
PR_Github #61112 [ run ] triggered by Bot. Commit: |
|
PR_Github #61112 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61118 [ run ] triggered by Bot. Commit: |
|
PR_Github #61118 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61127 [ run ] triggered by Bot. Commit: |
4db94a3 to
861456c
Compare
|
/bot kill |
|
/bot run |
|
PR_Github #61132 [ kill ] triggered by Bot. Commit: |
|
PR_Github #61134 [ run ] triggered by Bot. Commit: |
|
PR_Github #61132 [ kill ] completed with state |
|
PR_Github #61127 [ run ] completed with state |
|
PR_Github #61134 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61186 [ run ] triggered by Bot. Commit: |
|
PR_Github #61186 [ run ] completed with state
|
…ce fusion kernels The custom oneshot/twoshot allreduce fusion kernels accumulate in the native 16-bit dtype by default (use_fp32_acc: off unless ALL_REDUCE_FUSION_KERNEL_ACC_FP32 is set). On models with large residual-stream outliers (GPT-OSS 120B) this costs measurable accuracy whenever the autotuner selects the ONESHOT/TWOSHOT tactics over NCCL variants: with identical pinned autotuner tactics, enabling FP32 accumulation recovers +3.5 GPQA points end-to-end (62.121 -> 65.657). Flip the default to FP32 accumulation; opt out with ALL_REDUCE_FUSION_KERNEL_ACC_FP32=0 (legacy behavior, bit-preserved). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Dongfeng Yu <dongfengy@nvidia.com>
861456c to
b4be9b9
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #61473 [ run ] triggered by Bot. Commit: |
|
PR_Github #61473 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61528 [ run ] triggered by Bot. Commit: |
|
PR_Github #61528 [ run ] completed with state
|
Description
Fixes the intermittent GPT-OSS 120B accuracy failures on DGX-B200 tracked in https://nvbugs/6450338 (
TestGPTOSS::test_w4_4gpus[v1_kv_cache-ep4-trtllm-fp8], GPQA 55.051 < 55.734).The custom oneshot/twoshot allreduce fusion kernels accumulate in the native 16-bit dtype by default (
use_fp32_acc()is off unlessALL_REDUCE_FUSION_KERNEL_ACC_FP32is set). The autotuner (allreduce_strategy=AUTO) picks tactics on speed alone, so machine state during profiling decides whether the eval runs on the bf16-accumulating ONESHOT/TWOSHOT kernels or on fp32-accumulating NCCL variants. On models with large residual-stream outliers (GPT-OSS 120B, residual values up to ~75k where bf16 ulp is 512) this is a ~5-point GPQA swing, and the CI failure is the tail of that lottery.This PR flips the default to FP32 accumulation, matching NCCL and the newer MNNVL kernels (which accumulate in fp32 unconditionally).
ALL_REDUCE_FUSION_KERNEL_ACC_FP32=0opts out and is bit-identical to the legacy behavior.Measurements (all on B200 tp4, autotuner cache pinned so only the accumulation dtype changes):
696904a6d387: GPQA 62.121 → 65.657 with fp32 accumulation on the ONESHOT assignment (bit-stable across repeats); GSM8K 89.992 → 90.675.mpirun -n 4 python tests/microbenchmarks/all_reduce.py --benchmarkA/B (fp32 vsALL_REDUCE_FUSION_KERNEL_ACC_FP32=0, same machine, back-to-back): across all 36 rows that hit the modified kernels (ONESHOT/TWOSHOT × 4 fusion patterns × 256B–256MB), fp32 ranges from 4% faster to 2% slower (average ~1% faster), inside the ±3% run-to-run spread of the untouched strategies (NCCL/NCCL_SYMMETRIC/MNNVL/UB). No perf regression.Test Coverage
accuracy/test_llm_api_pytorch.py::TestGPTOSS::test_w4_4gpus[v1_kv_cache-ep4-trtllm-fp8](the failing test from the bug), run pinned on two B200 machines × three autotuner-cache states.tests/microbenchmarks/all_reduce.py --benchmarkfor the perf A/B above.PR Checklist
🤖 Generated with Claude Code
Dev Engineer Review
use_fp32_acc()incpp/tensorrt_llm/kernels/communicationKernels/allReduceFusionKernels.cuso FP32 accumulation is now the default.ALL_REDUCE_FUSION_KERNEL_ACC_FP32is unset or its first character is not'0'; settingALL_REDUCE_FUSION_KERNEL_ACC_FP32=0restores the legacy behavior (native-dtype accumulation).QA Engineer Review
No test changes.