Enable softmax-sink FMHA kernels for TransformerEngine - #7
Open
shurale-nkn wants to merge 8 commits into
Open
Conversation
Collaborator
|
@shurale-nkn have you opened PR's to the upstream CK library (which is part of the rocm-libraries repo) for these patches yet? Ideally it would be reviewed there for correctness, and we'd accept the patch here off of that review. |
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Required by ROCm/TransformerEngine#678 — [ROCm] Jax Add softmax sink (learnable
off-by-one) support for the ROCm/CK fused attention backend.
This PR is the TransformerEngine-side mirror of ROCm/rocm-libraries#10519 ([CK] [MHA] Fix for MHA with softmax-sink), delivered as a QoLA patch because the CK snapshot
vendored by our pinned AITER commit predates that fix.
Important
The patch is temporary. Once CK 0282f870b propagates into the AITER branch we build against,
3rdparty/QoLA/patches/aiter/0009-fmha-sink-fixes.patchcan simply be deleted — noreplacement, no follow-up. Nothing else in TE depends on it.
On the AITER side the related change is PR #4612, but that one only touches verification —
the API and the runtime behaviour are unchanged, so it has no bearing on this patch.
Note that #10519 is deliberately larger than what we carry here: it also contains changes that
no TE call path can reach. See Not included, and why.
Shipped
/patches/aiter/0009-fmha-sink-fixes.patch— a single patch, code only, no commentsin the hunks. It supersedes the former
0008/0009/0010and carries three changes:kernel/fmha_bwd_kernel.hppsink_ptrindexed byi_nheadinstead ofi_batch * nhead + i_nheadd_softmax_offsetand the q/k/v gradients for every batch index > 0pipeline/block_fmha_pipeline_qr_ks_vs.hpp(bias, randval, K/V) and..._async.hpp(bias, randval)i_total_loops == 0QRKSVS, so forward output was garbage (rel. err ~1.0) for bias + SWA + sinkkernel/fmha_fwd_kernel.hppGetSoftmaxScale()instead of rawkargs.scale_sNO_SCALETE reaches all three through
softmax_offset(softmax_type != "vanilla"), which setshas_sinkwithsink_size = 0.The first two were already shipped as separate patches and are unchanged in behaviour — this PR
only merges them and strips the C++ comments they were injecting into the CK sources. The third is
new and pre-emptive: it is a verified no-op today, carried because its gate can open without
any TE source change. Receipt 700 (our own patch
0003) is what currently keepsPERTENSORinstances out of the build, and it is rebased on every AITER bump.Not included, and why
All of the following are present in #10519 and were deliberately left out of the QoLA patch,
because no TE call path reaches them.
Async K/V sink-prefix jump. Turns a dead branch (
i_total_loops == 0sits afteri_total_loops++) into one guarded bynum_sink_loop > 0. TE pinssink_size = 0, so it neverfires either way — zero behavioural difference.
MX scale-window offsets. Not gated on MX, gated on
sink_size > 0. The two hunks that lookunconditional (
seqlen_k_start→kv_load_starton the scale-window origins) are arithmeticno-ops when
sink_seq_end == 0, becausekv_load_start = (sink_seq_end == 0 && seqlen_k_start > 0) ? seqlen_k_start : 0makes the twoexpressions equal in every branch. The rest sit inside the
i_total_loops == num_sink_loop - 1guard, which evaluates to
== -1whennum_sink_loop == 0. The two async hunks additionally haveno target: that pipeline declares no scale windows and carries
static_assert(QScaleEnum != BlockAttentionQuantScaleEnum::MX). Since quantization is the wronggate here, this stays irrelevant even after fp8/mxfp land.
Host reference and tests.
fmha_{fwd,bwd}_runner.hppandtest_fmha_{fwd,bwd}.cppareincluded only by CK's
example_fmha_*.cppandtest_fmha_*.cpp, none of which QoLA builds — itbuilds
libmha_fwd/libmha_bwdfrom generated blobs pluscpp_itfssources only.Comment rewrites. Comment-only upstream churn. The merged patch carries no comments at all;
the rationale lives in the patch message instead, where it does not have to be rebased.
Technical Details
Test Plan
Test Result
Submission Checklist