[None][perf] Size the per-tensor FP8 dynamic-quant amax grid to the input - #16569
Conversation
📝 WalkthroughWalkthroughChangesFP8 Dynamic Quantization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant FP8QDQLinearMethod
participant fp8_dynamic_quantize
participant TritonScaleReduction
participant static_quantize_e4m3_per_tensor
FP8QDQLinearMethod->>fp8_dynamic_quantize: quantize dynamic activation
fp8_dynamic_quantize->>TritonScaleReduction: compute scale
TritonScaleReduction-->>fp8_dynamic_quantize: return scale
fp8_dynamic_quantize->>static_quantize_e4m3_per_tensor: quantize input with scale
static_quantize_e4m3_per_tensor-->>FP8QDQLinearMethod: return quantized input and scale
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@tensorrt_llm/_torch/modules/fp8_dynamic_quant_triton.py`:
- Around line 81-85: Add Google-style Args and Returns sections to both public
helpers, fp8_dynamic_quant_scale and fp8_dynamic_quantize, documenting their
arguments and return values. Update fp8_dynamic_quantize’s signature to annotate
its return type as tuple[torch.Tensor, torch.Tensor], using Python 3.10 built-in
generic syntax.
- Around line 52-62: Update the FP8 dynamic-quantization path around
FP8QDQLinearMethod.apply and _amax_partial_kernel to handle strided 2-D inputs
before the pointer-linear reduction. Either make the reduction operate on a
contiguous representation of the logical input or pass and apply the input
strides when loading elements, ensuring views such as x[:, ::2] include exactly
their logical values and exclude skipped storage.
- Line 61: Update the accumulator reduction in the Triton quantization path near
acc and quantize_e4m3_per_tensor so NaN inputs receive the same handling as the
CUDA implementation; either configure the maximum operation to propagate NaNs
explicitly or add a regression parity test covering NaN-containing inputs before
claiming bitwise identity.
🪄 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: 2bd2fb03-9a92-42f7-bc59-2c77e76a2a96
📒 Files selected for processing (2)
tensorrt_llm/_torch/modules/fp8_dynamic_quant_triton.pytensorrt_llm/_torch/modules/linear.py
f77c236 to
e460a9d
Compare
|
/bot run --disable-fail-fst |
|
PR_Github #60254 Bot args parsing error: usage: /bot [-h] |
kaiyux
left a comment
There was a problem hiding this comment.
Should we move this file to tensorrt_llm/_torch/modules/fused_ops?
|
/bot run --disable-fail-fast |
|
PR_Github #60278 [ run ] triggered by Bot. Commit: |
c0e43f0 to
04b89ce
Compare
|
PR_Github #60278 [ run ] completed with state
|
Done. |
04b89ce to
d25d8e0
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #60347 [ run ] triggered by Bot. Commit: |
The dynamic-quantization path of FP8QDQLinearMethod computes the per-tensor amax with torch.ops.tensorrt_llm.quantize_e4m3_per_tensor, whose amax half (computeFP8QuantizeScale in cudaFp8Utils.cu) is a shared-memory block reduce that is SM-underutilized / sync-bound (~6us) at skinny decode shapes (M=64) and serializes ahead of the projection GEMM across the ~30 GDN layers. Replace ONLY the amax->scale computation with a fused Triton per-tensor kernel (1.84x vs the CUDA fused op under CUDA graph), keeping the same apply-only quantize op. The scale and quantized output are bitwise-identical to torch.ops.tensorrt_llm.quantize_e4m3_per_tensor, so this is a pure throughput change with no accuracy impact (GSM8K unchanged within noise). Measured on Qwen3.6-35B-A3B-NVFP4, B200, ISL 1024 / OSL 6144, 9-point concurrency sweep: +2.5% (concurrency 256) to +14.7% (concurrency 1) output throughput; +5.4-5.6% at the canonical high-concurrency operating point. Signed-off-by: Yukun He <23156053+hyukn@users.noreply.github.com>
d25d8e0 to
0631b79
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #60363 [ run ] triggered by Bot. Commit: |
|
PR_Github #60347 [ run ] completed with state |
|
PR_Github #60363 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #60452 [ run ] triggered by Bot. Commit: |
|
PR_Github #60452 [ run ] completed with state |
JunyiXu-nv
left a comment
There was a problem hiding this comment.
LGTM from runtime side.
Supersede the Triton FP8 dynamic-quant amax (previous commit) with a fix to the in-tree CUDA kernel it was working around. The dynamic per-tensor path (invokeComputeScalesAndQuantizeMatrix / invokeComputeFP8QuantizeScale) launched computeFP8QuantizeScale<PER_TENSOR> with a FIXED grid(1024) and reduced across blocks with a 1024-way atomicMax to a single scale address. On a skinny decode activation ([64,2048]) only ~128 of the 1024 blocks load data; the rest launch, block-reduce zeros, and still contend on the one atomic — a data-independent ~6us floor. Fix: size the amax grid to the input via the existing scaleMatrixVecGridSize(), and add a vectorized PER_TENSOR amax kernel (128-bit loads) mirroring the existing vectorized apply kernel. This reverts the Python/Triton change (fp8_dynamic_quant.py, linear.py, its unit test and l0 entry); the previous commit is kept in history. Why the CUDA fix over the Triton replacement: - Per-tensor amax is exact (max reduction), so the op output is bitwise- identical to baseline (verified: scale + quantized bytes across M=1..65536; GSM8K 96.75 vs 96.25 baseline, within noise). - It is uniformly >= the Triton kernel and does NOT regress at large M (prefill), which the fixed-grid-128 Triton kernel did (crossover ~M=2048). - E2E (Qwen3.6-35B-A3B-NVFP4, B200, ISL1024/OSL6144, concurrency 1..256): mean +7.34% vs baseline, vs +4.87% for the Triton version. - One kernel for prefill and decode; no new Triton dependency. Signed-off-by: Yukun He <23156053+hyukn@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #60697 [ run ] triggered by Bot. Commit: |
|
PR_Github #60697 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #60826 [ run ] triggered by Bot. Commit: |
|
PR_Github #60826 [ run ] completed with state |
…nput (NVIDIA#16569) Signed-off-by: Yukun He <23156053+hyukn@users.noreply.github.com>
Description
The per-tensor FP8 (e4m3) dynamic quantization path (
invokeComputeScalesAndQuantizeMatrixandinvokeComputeFP8QuantizeScaleincudaFp8Utils.cu) computes the amax withcomputeFP8QuantizeScale<PER_TENSOR>, launched with a fixedgrid(1024)and reducing across blocks with a 1024-wayatomicMaxto a single scale address. On a small activation only a fraction of the 1024 blocks load any data — the rest still launch, block-reduce zeros, and contend on the single atomic — so the kernel has a data-independent floor (~6–8us) no matter how little it actually processes (~200x off the memory-bound time for a 256KB input). This is a property of the launch/reduction design, not the data, and it applies to any model that uses the dynamic per-tensor FP8 quant path.This PR sizes the amax grid to the input via the existing
scaleMatrixVecGridSize(numel)(as thePER_CHANNELand the vectorized apply paths already do), and adds a vectorizedPER_TENSORamax kernel (computeFP8QuantizeScalePerTensorVec, 128-bit loads / 8 bf16 per thread) mirroring the existing vectorized apply kernel. The net change iscpp/tensorrt_llm/common/cudaFp8Utils.cuonly.This supersedes the earlier Triton-based approach in this PR (first commit, kept in history): fixing the in-tree kernel is uniformly ≥ a Triton replacement, needs no new dependency, and serves both prefill and decode with one kernel.
Test Coverage
Kernel microbench — per-tensor amax only, B200, under CUDA graph (us/call), original fixed-grid + 1024-way-atomic vs this fix (numel-sized grid + vectorized):
The fix removes the fixed ~6–8us floor at skinny (small-M) shapes and, via the vectorized loads, is also ~2x faster at very large M — i.e. no large-batch (prefill) regression.
Correctness. The per-tensor amax is an exact reduction and the scale is computed with the same
amax / 448, so the op output is bitwise-unchanged. Verified on B200 against a torch reference forM = 1 … 65536: the returned scale is bitwise-equal and the quantized bytes match the apply-only op at every shape. Existing FP8-quantization tests cover the op path.PR Checklist