blockwise fp8 gemm integration for gfx942 and gfx950#658
Conversation
…dant HIP guards, revert unnecessary common.h change
# Conflicts: # tests/cpp/operator/CMakeLists.txt
…hipkittens_optim # Conflicts: # transformer_engine/common/CMakeLists.txt # transformer_engine/common/gemm/kittens/CMakeLists.txt # transformer_engine/common/gemm/rocm_gemm.cu # transformer_engine/pytorch/quantization.py
…gemm_hipkittens_optim # Conflicts: # transformer_engine/common/gemm/kittens/cdna4/mxfp8_gemm.cpp # transformer_engine/pytorch/quantization.py
b6a9239 to
2df055e
Compare
2df055e to
3b3bb80
Compare
Claude WalkthroughIntent. Adds blockwise FP8 GEMM support on ROCm for both gfx942 (CDNA3) and gfx950 (CDNA4) using HipKittens kernels. Extends the prior gfx950-only HipKittens MXFP8 path with new blockwise-scaled FP8 kernels (1Dx1D and 1Dx2D scaling), and pulls in a separate HipKittens submodule for CDNA3 since the two archs use different kernel branches. Key changes.
Walkthrough. Kernel layer. Two independent HipKittens builds are kept side-by-side because CDNA3 and CDNA4 need different HipKittens branches. Each arch has its own CMake. Previously the kittens lib was gfx950-only. It is now a function that conditionally adds per-arch OBJECT libraries ( rocm_gemm.cu integration. The blockwise path is checked before mxfp8/hipblasLt. It enforces: output must be bf16/fp32/fp16; B must be 1D-scaled (so only 1Dx1D or 1Dx2D permitted); TT layout is rejected; e5m2 x e5m2 is rejected; gelu epilogue requires Python side. The prior split between "quantization available" (cast) and "GEMM available" (kernels) is collapsed since ROCm now has both. Testing. No new test files. Existing Notes for reviewers.
Generated by Claude. To request a code review, comment |
| [submodule "3rdparty/hipkittens_cdna3"] | ||
| path = 3rdparty/hipkittens_cdna3 | ||
| url = https://github.com/asdfvg123/HipKittens.git | ||
| branch = yeonsoo/cdna3_fp8 |
There was a problem hiding this comment.
Submodule points to a personal user fork on an unreleased branch — must be relocated before merge.
url = https://github.com/asdfvg123/HipKittens.gitis a personal repo, not a canonical one. The sibling3rdparty/hipkittenscorrectly points atHazyResearch/HipKittens.git. Anyone cloning ROCm/TransformerEngine will failgit submodule update --initthe moment this repo goes private or gets renamed. It also blocks reproducible ROCm CI wheel builds (this submodule is now in the init list in.github/workflows/rocm-wheels-build.yml).- The pinned branch
yeonsoo/cdna3_fp8is likewise a personal in-flight branch; the CDNA3 kittens sources need to land in a persistent upstream branch (HazyResearch or a ROCm-owned mirror) and this URL/branch should be updated before merge.
| NVTE_CHECK((m % 8) == 0, | ||
| "Blockwise FP8 GEMM requires N divisible by 8"); | ||
| NVTE_CHECK(inputB->scaling_mode != NVTE_BLOCK_SCALING_1D || (n % 16) == 0, | ||
| "Blockwise FP8 GEMM requires M divisible by 16 for 1D scaling"); |
There was a problem hiding this comment.
Error messages have M/N swapped relative to the values being checked.
The check is on m but the message says "N divisible by 8", and the check on n says "M divisible by 16". A user hitting either of these gets a message that names the wrong dimension, which will be very confusing to debug (especially since the arch impls internally swap kM=N/kN=M — a reader seeing the message will not know which side of the swap is being enforced).
Fix: name the dimension after the value actually being checked, e.g.
| NVTE_CHECK((m % 8) == 0, | |
| "Blockwise FP8 GEMM requires N divisible by 8"); | |
| NVTE_CHECK(inputB->scaling_mode != NVTE_BLOCK_SCALING_1D || (n % 16) == 0, | |
| "Blockwise FP8 GEMM requires M divisible by 16 for 1D scaling"); | |
| NVTE_CHECK((k % 16) == 0, | |
| "Blockwise FP8 GEMM requires K divisible by 16"); | |
| NVTE_CHECK((m % 8) == 0, | |
| "Blockwise FP8 GEMM requires M divisible by 8"); | |
| NVTE_CHECK(inputB->scaling_mode != NVTE_BLOCK_SCALING_1D || (n % 16) == 0, | |
| "Blockwise FP8 GEMM requires N divisible by 16 for 1D scaling"); |
(If the intent was to speak in the kernel's post-swap convention, then the two messages need to be swapped with each other — either way the current pairing is wrong.)
Also note: since the preceding NVTE_CHECK(inputB->scaling_mode == NVTE_BLOCK_SCALING_1D, ...) already forces B to be 1D, the inputB->scaling_mode != NVTE_BLOCK_SCALING_1D || guard on the last check is always false-branch — you can drop it and just check (n % 16) == 0 unconditionally.
| if IS_HIP_EXTENSION and get_device_compute_capability() in ((9, 4), (9, 5)): | ||
| expected_err_msg = None | ||
| expected_err_cls = RuntimeError |
There was a problem hiding this comment.
This weakens the "constraint enforced" tests to a near-tautology on ROCm.
Setting expected_err_msg = None here means the test now accepts any RuntimeError from the ROCm code path, regardless of whether it comes from the constraint we're supposedly enforcing or from an unrelated failure (setup error, shape mismatch, OOM, etc.). Every call site of cublas_gemm_test_constraint_enforced is paired with a specific constraint (illegal dtype, illegal 2Dx2D, split-accumulator, bgrad, gelu-unsupported…), and none of that specificity is preserved on ROCm.
Prefer either:
- Mapping the cuBLAS-flavored
expected_err_msgto the corresponding ROCm one (theNVTE_CHECKstrings you added inrocm_gemm.cuabove, e.g."Blockwise FP8 GEMM does not support ..."), so the test still asserts the right thing, or - Passing a per-callsite ROCm expected message via the caller instead of blanket-nulling it here.
| if key in _FP8_KEYS: | ||
| value = ( | ||
| get_torch_float8_e4m3_type() if key is tex.DType.kFloat8E4M3 | ||
| get_torch_float8_e4m3_type() if key == tex.DType.kFloat8E4M3 |
There was a problem hiding this comment.
This change also affects the CUDA path — please classify and document.
Custom_DType_Dict is shared code that both CUDA and ROCm execute. Changing is to == on the pybind11 enum is likely a real bug fix (enum identity via is is brittle across module reloads / repeated pybind imports), not a ROCm-specific tweak. Per the ROCm-fork upstream-compat rule, generic bug fixes to shared code are fine but should be called out explicitly (PR description or a short comment) so it can be upstreamed and future IFU merges know why the line diverges from NVIDIA's TE.
If instead this change is only motivated by a ROCm-observed failure, please add a one-line comment explaining that, or guard it with IS_HIP_EXTENSION.
| gpu_arch = get_device_compute_capability() | ||
| if gpu_arch in ((9, 4), (9, 5)): # TODO: enabled for gfx1250 when ready | ||
| return True, "" | ||
| return False, "Device arch gfx94x or newer is required for FP8 block scaling execution." |
There was a problem hiding this comment.
This drops is_fp8_block_scaling_quantization_available — that is a public API change.
is_fp8_block_scaling_quantization_available and the underlying check_fp8_block_scaling_quantization_support were being removed together with the corresponding re-export from transformer_engine/pytorch/__init__.py and the __all__ entry above. That symbol was exposed on transformer_engine.pytorch, so any downstream code (or ROCm-side tests / benchmarks / megablocks / megatron patches) that imported is_fp8_block_scaling_quantization_available(...) will now break with ImportError.
Two options:
- If block scaling GEMM being available implies block scaling quantization is available (the new
_compute_fp8_block_scaling_supportgating on(9,4)/(9,5)suggests yes), keep the old API as a thin alias tois_fp8_block_scaling_availableand mark it deprecated, so we don't break importers. - If we're intentionally taking the API away, please call this out in the PR description as a breaking change.
Claude review — PR 658Reviewed the ROCm-only 3-dot diff ( Verdict: solid direction, but there are a few things worth fixing before merge. Findings posted inline:
Copyright headers: OK — every touched file carries an up-to-date AMD 2026 header in the correct format; NVIDIA lines preserved on modified upstream files. |
|
Description
Integrating blockwise FP8 gemm for gfx942 and gfx950 using hipkittens. Using 2 submodules for now. The hipkittens submodule uses separate branch for CDNA3 and CDNA4.
Fixes # (issue)
Type of change
Changes
Please list the changes introduced in this PR:
separate the kernel for gfx942 and gfx950. The host selects the kernel in runtime.
The kernel supports 1d2d, 1d1d GEMM with output bf16, fp32, fp16 / input fp8e5m2, fp8e4m3 / and different epilogues.
Checklist: