Align Hopper FP8 blockwise quant to power-of-2 scales; fix tl.trans#74
Align Hopper FP8 blockwise quant to power-of-2 scales; fix tl.trans#74haok1402 wants to merge 8 commits into
Conversation
|
@claude review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Triton quantization kernels in deepgemm_quantize.py to use E8M0 power-of-2 scaling factors across both Hopper (SM90) and Blackwell (SM100+) architectures, while renaming the public functions to use a cleaner fp8cast_ prefix. Additionally, it addresses a Triton bug with tl.trans on FP8 tiles by writing to swapped-index addresses. The review feedback highlights that this swapped-index approach results in uncoalesced global memory stores, which can degrade performance. It is recommended to instead perform the transposition in float32 before casting to fp8 to maintain coalesced memory writes while still avoiding the Triton bug.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Benchmarked the three transposed-store strategies on an idle H100 ( All three within ~1%. No regression. Also compared end-to-end training throughput (dsv2-lite, 64 steps, steady-state): +0.81% after. Within noise. No regression at the training level either. |
Fixed. See https://wandb.ai/pithtrain/hopper-deepgemm/workspace?nw=28pacu7obnf |
The fused_ prefix carried no information (every kernel in the module is fused), so drop it and adopt DeepGEMM's "cast" verb as an fp8cast_ prefix; also drop the leading underscore on the module-internal kernels/helper. Update all call sites in linear, grouped_linear, gpt_oss, ring_attention, and the kernel test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
73a7dcb to
e5ab647
Compare
|
@claude review |
Performance ReviewNo performance issues found. The only perf-bearing change is the blockwise-transpose kernels dropping |
Compactness ReviewNo compactness issues. This PR is net-subtractive on surface: it replaces the |
Consistency ReviewThe rename ( One stale reference, outside this PR's diff so it has no inline thread: |
| # --- Blockwise transpose path (reuse tok_amax) --- | ||
| block_amax = tl.max(tok_amax, axis=0) # scalar | ||
| blk_scale, blk_rcp = _compute_fp8_scale(block_amax, SCALING_MODE) # scalar | ||
| blk_scale, blk_rcp = _compute_fp8_scale(block_amax, NATIVE_E8M0) # scalar |
There was a problem hiding this comment.
Correctness: A few lines below (line 558), this kernel still transposes an fp8 tile with tl.trans(blk_fp8) — the exact pattern the PR's own minimal repro shows corrupts (4090/16384 mismatches on H100 / Triton 3.7.1) and that the two blockwise kernels were rewritten to avoid. The same hazard also remains in _fp8cast_rowwise_transpose_kernel at line 414 (tl.trans(t_fp8)).
Both functions feed the transposed FP8 (input_t_fp8 / grad_t_fp8 / x_t_fp8) into the dgrad/wgrad GEMMs (linear.py:54,76, ring_attention.py:645), so a miscompile here silently corrupts gradients while forward/loss stay clean.
The unit suite passes for these two, so the trigger is narrower than "tl.trans corrupts fp8 tiles" as stated — plausibly it fires only when the same fp8 SSA value is both stored directly and transposed (as in the fixed kernels, which reuse fp8_tile), whereas here a distinct value feeds tl.trans. That distinction is codegen-dependent: a Triton/register-allocation change could flip these to corrupt with no test signal.
Please either apply the same swapped-index store here and at line 414, or document the precise trigger and extend the repro to this two-distinct-values shape to prove immunity. (Couldn't verify empirically — no CUDA in this review env.)
Correctness ReviewScope: rename One correctness concern (inline): the PR's stated root cause — "Triton 3.7.1 Evidence is otherwise solid: 48 bit-exact unit tests plus the committed wandb dsv2-lite FP8 loss curve cover the behavior-affecting path. |

TLDR: Brings the Hopper FP8 path onto the same power-of-2 block scaling that NVIDIA TransformerEngine uses by default. In TE, fp32-mantissa scales are a Hopper-only opt-out (
NVTE_FP8_BLOCK_SCALING_FP32_SCALES=1) and Blackwell is pow2-only. See FP8 Blockwise Scaling.What changed
tl.transcorruptsfp8tiles, producing a wrong transposed FP8 weight (used in dgrad). Fix: store to transposed addresses instead oftl.transon fp8.fused_* -> fp8cast_*: every kernel in this module is fused, sofused_said nothing;fp8cast_uses DeepGEMM's own "cast" verb.SCALING_MODE->NATIVE_E8M0(it just means native-SM100 vs emulated-SM90 E8M0; both are pow2) and corrected the docstrings.tests/test_fp8_quantize_kernels.py->tests/operators/test_deepgemm_quantize.py; fixed the reference to always use E8M0 (was previously device-gated and wrong on Hopper -> 39 failures).Validation
pytest tests/operators/test_deepgemm_quantize.py-> 48 passed (was 39 failing on H100).workspace/triton_fp8_trans_repro.py, included below):kernel1(tl.trans) mismatches 4090/16384;kernel2(transposed-address store, the fix) passes with 0.Files
pithtrain/operators/deepgemm_quantize.pytests/operators/test_deepgemm_quantize.pyReproduction of the Triton bug