[Common/PyTorch] Fused grouped MXFP8 requantization - #3359
Conversation
Replace the group_dequantize -> group_quantize(columnwise) -> grouped_swizzle(rowwise scales) chain in group_requantize_inplace with a single kernel (NVTE_FUSED_GROUP_REQUANTIZE=0 restores the unfused path). Co-authored-by: Oleg Goncharov <ogoncharov@nvidia.com> Signed-off-by: YangFei1990 <feiw@nvidia.com>
Greptile SummaryThe PR replaces grouped MXFP8 dequantize, columnwise requantize, and scale swizzle operations with a fused Blackwell CUDA kernel while retaining an environment-controlled fallback.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains in the eligible follow-up-review scope. No blocking failure remains. Important Files Changed
Sequence DiagramsequenceDiagram
participant PT as PyTorch GroupedTensor
participant Bind as group_requantize_inplace
participant Core as nvte_group_requantize
participant GPU as Fused CUDA kernel
PT->>Bind: Rowwise MXFP8 data, scales, offsets
alt Fused path supported and enabled
Bind->>Core: Input/output descriptors and offsets
Core->>GPU: Launch grouped requantization
GPU-->>Core: Columnwise E4M3 data and swizzled scales
Core-->>Bind: Optional BF16 dequantized output
Bind-->>PT: Replace scale and columnwise buffers
else Fallback path
Bind->>Bind: Group dequantize
Bind->>Bind: Group columnwise quantize
Bind->>Bind: Grouped scale swizzle
end
Reviews (3): Last reviewed commit: "[pre-commit.ci] auto fixes from pre-comm..." | Re-trigger Greptile |
Signed-off-by: YangFei1990 <feiw@nvidia.com>
…90/TransformerEngine into fused_group_requantize
Oleg-Goncharov
left a comment
There was a problem hiding this comment.
Hi @YangFei1990, I’ve done some further optimization work on this implementation, and the version in this GitLab branch is ~10% faster than the current implementation in this PR:
https://gitlab-master.nvidia.com/ogoncharov/transformerengine/-/tree/pr_requantize_mxfp8
The main improvements came from using data swizzling in the TMA descriptor and the redux.sync instruction, which reduces the number of reads/writes to the shared buffer holding intermediate results.
Could you please update the PR to incorporate the corresponding changes before merging?
| * \param[in] stream CUDA stream used for the operation. | ||
| */ | ||
| void nvte_fused_group_requantize_mxfp8(const NVTETensor input, NVTETensor output, | ||
| const NVTETensor tensor_offsets, NVTETensor dequantized, |
There was a problem hiding this comment.
In which case do callers need the dequantized?
Could we rename this function to nvte_group_requantize since it already contains "fusion" and is also more general, so that we can extend it in the future? Ofc we should make sure to check and only support MXFP8 for now.
There was a problem hiding this comment.
dequantized is required to compute dbias when it is needed. I will update the func name.
| * path. | ||
| * \param[in] stream CUDA stream used for the operation. | ||
| */ | ||
| void nvte_fused_group_requantize_mxfp8(const NVTETensor input, NVTETensor output, |
There was a problem hiding this comment.
Could we leave out the "mxfp8" from the name? We can note in the documentation that currently it only supports mxfp8, but we should still do a general API name.
| // unfused chain's dedicated empty-input handling accepts and the kernel's pointer | ||
| // validation (correctly) rejects. | ||
| const bool use_fused_kernel = | ||
| transformer_engine::getenv<bool>("NVTE_FUSED_GROUP_REQUANTIZE", true) && need_columnwise && |
There was a problem hiding this comment.
Why env variable? If we are confident of its perf, then we should always enable it.
There was a problem hiding this comment.
I would like to keep it just for debugging purpose. The default path is fused op. Please let me know if you really want to get rid of it.
| const bool use_fused_kernel = | ||
| transformer_engine::getenv<bool>("NVTE_FUSED_GROUP_REQUANTIZE", true) && need_columnwise && | ||
| has_usable_offsets && total_tokens > 0 && otype == DType::kBFloat16 && | ||
| quantizer.attr("dtype").cast<DType>() == DType::kFloat8E4M3 && |
There was a problem hiding this comment.
Doing attr access for the same attribute again and again should be avoided, since it can be pretty heavy considering cpu overheads.
Could we initialize them once for the fused and unfused cases in the start of the function and reuse them?
| "Requantizing a grouped input requires dims that are multiples of 128, but got (", | ||
| total_tokens, ", ", hidden_dim, ")."); | ||
|
|
||
| // Fused path (default; NVTE_FUSED_GROUP_REQUANTIZE=0 recovers the unfused chain): one |
There was a problem hiding this comment.
Do we already have pytorch side unit test to test the fused kernel?
There was a problem hiding this comment.
Yes all tests in test_mxfp8_group_quantize_graph_safe.py will run this path
Oleg-Goncharov
left a comment
There was a problem hiding this comment.
The kernel LGTM for now. A follow-up PR with performance optimizations will come later
Signed-off-by: YangFei1990 <feiw@nvidia.com>
for more information, see https://pre-commit.ci
|
/te-ci L1 pytorch |
Based on the latest comment that the optimizations will be part of the subsequent PR.
Description
Replace the group_dequantize -> group_quantize(columnwise) -> grouped_swizzle(rowwise scales) chain in group_requantize_inplace with a single kernel (NVTE_FUSED_GROUP_REQUANTIZE=0 restores the unfused path).
Fixes # (issue)
Type of change
Changes
Please list the changes introduced in this PR:
Checklist: