[https://nvbugs/6525059][fix] Drop 8 from FP8BlockScaleMoeRunner::mSupportedTileN, covering both the… - #17135
[https://nvbugs/6525059][fix] Drop 8 from FP8BlockScaleMoeRunner::mSupportedTileN, covering both the…#17135trtllm-agent wants to merge 2 commits into
8 from FP8BlockScaleMoeRunner::mSupportedTileN, covering both the…#17135Conversation
|
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 skipped from review as they are similar to previous changes (2)
WalkthroughThe FP8 MoE runner no longer supports tileN=8. Supported tile sizes are 16, 32, 64, and 128. Three Qwen3 integration-test waivers are removed. ChangesFP8 MoE support and test waivers
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/tensorrt_llm/thop/fp8BlockScaleMoe.cpp (1)
367-373: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the supported tile sizes.
Line 373 uses non-exempt numeric literals directly in
mSupportedTileN. Replace them withk-prefixedconstexprconstants.Proposed change
- : mSupportedTileN{16, 32, 64, 128} + : mSupportedTileN{kSupportedTileN16, kSupportedTileN32, kSupportedTileN64, kSupportedTileN128} private: + static constexpr int32_t kSupportedTileN16{16}; + static constexpr int32_t kSupportedTileN32{32}; + static constexpr int32_t kSupportedTileN64{64}; + static constexpr int32_t kSupportedTileN128{128};As per coding guidelines, avoid magic literals except
0,nullptr,true, andfalse, and usek-prefixed camelCase constants.🤖 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/thop/fp8BlockScaleMoe.cpp` around lines 367 - 373, Define k-prefixed constexpr constants for the supported tile sizes near the relevant configuration declarations, then initialize mSupportedTileN with those named constants instead of the numeric literals 16, 32, 64, and 128. Preserve the existing supported-size set and ordering.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.
Inline comments:
In `@cpp/tensorrt_llm/thop/fp8BlockScaleMoe.cpp`:
- Around line 367-373: Validate cached and requested tile values against
mSupportedTileN before runner lookup in the relevant run() path, so unsupported
tileN=8 configurations are rejected or invalidated instead of reaching
mRunners.at(). Preserve valid cached configurations and the existing supported
tile set.
---
Nitpick comments:
In `@cpp/tensorrt_llm/thop/fp8BlockScaleMoe.cpp`:
- Around line 367-373: Define k-prefixed constexpr constants for the supported
tile sizes near the relevant configuration declarations, then initialize
mSupportedTileN with those named constants instead of the numeric literals 16,
32, 64, and 128. Preserve the existing supported-size set and ordering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3cdeff4c-99e0-47d4-91f8-d7821162500c
📒 Files selected for processing (2)
cpp/tensorrt_llm/thop/fp8BlockScaleMoe.cpptests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
|
/bot run --only-qa-verify |
|
PR_Github #63374 [ run ] triggered by Bot. Commit: |
03f178b to
b8db274
Compare
|
PR_Github #63374 [ run ] completed with state |
… MoE
The TRTLLM-Gen FP8 block-scale MoE picks tile_tokens_dim=8 whenever
num_tokens*top_k/local_num_experts <= 8, i.e. for warmup and tiny decode
batches. On sm100f/sm103 every DeepSeek-FP8 GEMM2 cubin at that tile
(bmm_Bfloat16_E4m3E4m3_Fp32_t128x8x128{,u2}_..._dsFp8_{schedS,schPd4x2x2x3}_bN_...)
faults with cudaErrorIllegalAddress in its TMA-OOB epilogue store, so the
warmup forward of Qwen3-30B-A3B-FP8 dies. Both the Static and the
Persistent variant fault, so the tile size is the discriminator rather
than a single bad cubin.
Remove 8 from the runner's supported tile list. This covers the autotuner
tactic list and the tileN == -1 fallback (which clamps on front()) in one
place, and avoids ever constructing the tileN=8 runner -- filtering the
cubins in TrtllmGenBatchedGemmRunner::skipQuirks instead would leave that
runner with no passing config and throw during construction. FP8
block-scale MoE stays enabled; small batches now run at tileN=16.
Unwaive the GB200/GB300 TestQwen3_30B_A3B::test_dummy_load_format entries.
Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
b8db274 to
05b8927
Compare
Summary
tile_tokens_dim=8faults withcudaErrorIllegalAddressin its TMA-OOB epilogue store, and tileN=8 is selected for warmup/tiny batches.8fromFP8BlockScaleMoeRunner::mSupportedTileN, covering both the autotuner tactic list and thetileN == -1fallback without constructing a config-less tileN=8 runner; unwaive the GB200/GB300 entries.Test plan
Links
Dev Engineer Review
tileN=8fromFP8BlockScaleMoeRunner::mSupportedTileN.tileN == -1fallback.tileN=8runner.cudaErrorIllegalAddressfaults on sm100f/sm103.TestQwen3_30B_A3B::test_dummy_load_format.QA Engineer Review
SKIPentries fromtests/integration/test_lists/waives.txt.03f178b.6525059.Verdict: sufficient