[JAX] [PyT] [Common] Enable D=256 BWD cuDNN fused attn for Blackwell CC 10.x #3056
[JAX] [PyT] [Common] Enable D=256 BWD cuDNN fused attn for Blackwell CC 10.x #3056KshitijLakhani wants to merge 28 commits into
Conversation
51ad582 to
d177ecf
Compare
Greptile SummaryThis PR extends Blackwell D=256 fused-attention backward support and its validation coverage.
Confidence Score: 3/5The PR does not yet appear safe to merge because THD D=256 backward may still be enabled before cuDNN can construct a supported execution plan. The current selector routes qualifying THD D=256 training calls on cuDNN 9.25 and newer into the cuDNN backend, but the prior thread and PR support statement leave unresolved whether this path is valid before cuDNN 9.30; a failed plan build would surface as a runtime exception rather than fallback. Files Needing Attention: transformer_engine/common/fused_attn/fused_attn.cpp Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
A["PyTorch / JAX D=256 training"] --> B["Common fused-attention backend selector"]
B --> C{"SM10x, cuDNN version, layout, and feature guards pass?"}
C -- No --> D["No fused backend / framework fallback"]
C -- Yes --> E["cuDNN F16 arbitrary-seqlen backend"]
E --> F["D=256 backward execution plan"]
Reviews (16): Last reviewed commit: "[pre-commit.ci] auto fixes from pre-comm..." | Re-trigger Greptile |
| attn_mask_type == NVTE_Mask_Type::NVTE_PADDING_CAUSAL_MASK || | ||
| attn_mask_type == NVTE_Mask_Type::NVTE_CAUSAL_BOTTOM_RIGHT_MASK || | ||
| attn_mask_type == NVTE_Mask_Type::NVTE_PADDING_CAUSAL_BOTTOM_RIGHT_MASK) && | ||
| (window_size_right == -1 || window_size_right == 0)))) || |
There was a problem hiding this comment.
Could these changes be moved to before "\ bias type" just so it's following an increasing order of the cuDNN version?
There was a problem hiding this comment.
Does this new feature support BSHD/SBHD and THD? It looks like the tests are focused on BSHD/SBHD only.
There was a problem hiding this comment.
RE: THD support
I did test BSHD and BSHD+CP and it did pass on the JAX side and the CI for the PyT side did not fail either so I think that works.
My testing revealed that THD support is not yet available (Bwd plan compialtion issue) so I've filed a bug and shared a reproducer for the same with the cuDNN team: NVIDIA/cudnn-frontend#276
There was a problem hiding this comment.
Could these changes be moved to before "\ bias type" just so it's following an increasing order of the cuDNN version?
Fixed in a264de1
| "D=256 BWD on Blackwell only supports right window -1 or 0" | ||
| " for causal masks." | ||
| ) | ||
|
|
There was a problem hiding this comment.
Aren't these checks duplicate to the checks we added on the C++ side? Would the call FusedAttnHelper().get_fused_attn_backend() give you the same gating effect?
There was a problem hiding this comment.
So if we are just interested in the gating effect, you are right. The get_fused_attn_backend() will return NVTE_No_Backend and then there's a catch-all at the end which basically skip the tests as there is no fused attn backend avalable.
However, the reason for this to be here is to give a meaningful reason as to why a test is being skipped as compared to a generic "Unsupported inputs combination or device compute capability." message which does not qualify the reason for the skip. Unfortunately, on the JAX attn side we do not log the reason for disabling fused attn in the feature code like we have on the Pytorch side in d_p_a/utils.py. So there is no way for the user to know why the test was skipped. Hence, we need to rely on test code to log this on the JAX side.
I'd suggest we leave this in here for now. And when your PR for generating log messages in the C++ level when selecting the attn backend is ready, I can plumb it through onto the JAX side and then as part of that clean up, get rid of all the skip messages in check_configs()
e317f99 to
e08e9e8
Compare
5063b39 to
55f3bb4
Compare
|
Latest testing: Pipeline [56553160] |
a678b84 to
ba0cb94
Compare
|
/te-ci L2 |
| // 9.23: d_qk = d_v = 256 + SM10x (cuDNN FE 1.24 / BE 9.23+) + bprop + non-paged. | ||
| // THD layouts require cuDNN FE 1.26 / BE 9.30+ for execution-plan support. | ||
| (head_dim_qk == 256 && head_dim_v == 256 && is_training && sm_arch_ >= 100 && | ||
| sm_arch_ < 110 && cudnn_runtime_version >= (is_thd_layout ? 93000 : 92300) && |
There was a problem hiding this comment.
The PR body says this should be 92500, not 93000. Which one is correct?
There was a problem hiding this comment.
Good catch !
I've updated the PR body to reflect cuDN 9.30. I believe the body was a preemptive action from me while I was waiting for cuDNN to add support for THD via the C++ API.
There was a problem hiding this comment.
I think cuDNN has renamed 9.30 to 9.25? Please double check!
There was a problem hiding this comment.
You are right indeed. Thanks for the note. I've made changes for the same
| # TODO(KshitijLakhani): cuDNN FE can model bias input separately from dBias, | ||
| # but TE does not yet plumb whether dBias is requested into the common backend selector. | ||
| # Until that distinction is available, the D=256 SM10x gate requires no bias. |
| // 9.23: d_qk = d_v = 256 + SM10x (cuDNN FE 1.24 / BE 9.23+) + bprop + non-paged. | ||
| // THD layouts require cuDNN FE 1.26 / BE 9.30+ for execution-plan support. | ||
| (head_dim_qk == 256 && head_dim_v == 256 && is_training && sm_arch_ >= 100 && | ||
| sm_arch_ < 110 && cudnn_runtime_version >= (is_thd_layout ? 93000 : 92300) && |
There was a problem hiding this comment.
Good catch !
I've updated the PR body to reflect cuDN 9.30. I believe the body was a preemptive action from me while I was waiting for cuDNN to add support for THD via the C++ API.
Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
for more information, see https://pre-commit.ci
…n fused attn Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
for more information, see https://pre-commit.ci
Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
…ersions Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
for more information, see https://pre-commit.ci
Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
for more information, see https://pre-commit.ci
Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
88217fd to
06eee35
Compare
Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
|
Latest testing: Pipeline [58978691] |
| "fused_hd256_padding_causal_gqa": ModelConfig( | ||
| 2, 1024, 16, 256, num_gqa_groups=4, attn_mask_type="padding_causal" | ||
| ), | ||
| } |
There was a problem hiding this comment.
Can we reuse some of the configs in other tests (I think there are some h256 ones as well), or can we keep this dict and remove those other h256 ones to deduplicate?
There was a problem hiding this comment.
I considered reusing the existing D=256 configs, but they are tied to other coverage: decode-style generic tests, mask/layout sweeps (using post scale bias which would be rejected by cuDNN D=256 bprop path), FA4, or MLA where d_v != 256 (hence not the same D=256 cuDNN kernel path either).
Changing them would either remove existing coverage or mix cuDNN D=256-specific gates into generic tests. I kept this as a small dedicated dict so the cuDNN FusedAttention D=256 bprop surface is explicit and isolated.
If you have a strong opinion on this, I can arm twist the code to do so
| "cp_hd256_causal": ModelConfig(2, 1024, 16, 256, attn_mask_type="causal"), | ||
| "cp_hd256_causal_swa": ModelConfig( | ||
| 2, 1024, 16, 256, attn_mask_type="causal", window_size=(128, 0) | ||
| ), |
There was a problem hiding this comment.
Should we test no_mask, padding_causal as well, or not really?
There was a problem hiding this comment.
I think it's okay to not test those two, if you are fine with it ?
The non-CP tests cover no_mask and I believe THD CP anyways translates to padding_causal under the hood, so I decided to keep this lightweight.
| ) | ||
| @pytest.mark.parametrize("dtype", ["bf16", "fp16"]) | ||
| @pytest.mark.parametrize("model,qkv_format,cp_comm_type", DPA_CP_FUSED_HD256_CASES) | ||
| def test_cp_with_fused_attention_hdim256(cp_pool, dtype, model, qkv_format, cp_comm_type): |
There was a problem hiding this comment.
Could we fold this into the regular test_cp_with_fused_attention() tests instead of creating this extra test code and extra tests? You can modify some of the existing tests to cover the d256 cases as well, or force some skips if you think it's adding too many tests (I feel your pytest params above might have enumerated all the combinations anyway). Thanks.
Also, please use d256 as the reference, same as in the title, instead of hdim256. (nit)
There was a problem hiding this comment.
Also, please use d256 as the reference, same as in the title, instead of hdim256. (nit)
Done. Do you want me to change it for the Flas Attn tests too ?
There was a problem hiding this comment.
Regarding folding this into the regular test_cp_with_fused_attention() tests, that was my first approach actually but I dropped it in favor of the current approach.
The current test_cp_with_fused_attention() is a pretty broad sweep which would add quite a few test cases (while still missing on quite a few as essential mode does not cover BSHD and FP16, both of which we want for D=256). Most of the collected test cases would just be skips adding unnecessary noise in the logs (e.g. fp8, diff mask combos, etc.) so I decided to take a more funneled approach.
The focused test is clearer: it directly covers BSHD and THD, fp16 and bf16, P2P and all-gather, plus THD SWA, without polluting the broad existing CP matrix.
Are you okay with it being as is @cyanguwa ?
There was a problem hiding this comment.
"essential mode does not cover BSHD and FP16, both of which we want for D=256"
Do we have SBHD tests in the essential mode instead? They should be equivalent to testing BSHD though.
I feel the D256 feature shouldn't be that special to have its own test function, even though the kernels are written in cutedsl, different from the regular SDPA. But we seem to be always expanding the range of head dims we support so do we want to add a new test every time, and would the test code become a bit repetitive.
There was a problem hiding this comment.
Do we have SBHD tests in the essential mode instead? They should be equivalent to testing BSHD though.
You are right about that
There was a problem hiding this comment.
I folded in the D=256 tests into the test_cp_with_fused_attention and added a bunch of skips in there for D=256 in commit 9921b99
This should cover most of the earlier D=256 tests I had added.
The only differences are:
- Older D=256 tests ran BF16 and FP16 in essential mode but the Newer D=256 tests only run BF16 (FP8 is not supported) and FP16 is ran as non-essential, which is consistent with other fused attn tests
- Older D=256 tests ran BSHD in essential mode but the Newer D=256 tests run SBHD instead, which is consistent with the other fused attn tests
So with this commit I think we are consistent throughout
| # cuDNN FE 1.26 / BE 9.30. The kernel rejects dBias, dropout, and ALiBi, supports vanilla | ||
| # softmax only, and allows SWA together with a causal mask only. | ||
| is_sm10x = 100 <= compute_capability < 110 | ||
| if self.is_training and is_sm10x and (self.head_dim_qk == 256 or self.head_dim_v == 256): |
There was a problem hiding this comment.
Shouldn't these checks already be covered on the C side? Are these by any chance duplicate?
There was a problem hiding this comment.
IIUC, is your concern the same as you had in your earlier review: #3056 (comment). ? I think my comment goes into more details. Happy to answer any follow ups on that.
I will get rid of these extraneous checks in the tests (if #2964 does not already) once #2964 is merged in. For now, I think we should let them be in. The clean up for these test messages can follow up once we've confirmed that a more descriptive string is plumbed through to the tests
There was a problem hiding this comment.
Sounds good - are there any CP specific skips/conditions we should add?
#2964 will surface the reject reason to Jax FusedAttnHelper.get_fused_attn_backend() so it'd be nice to use that as a single source of rejection/pass reasons. The d256 feature reject reason does come from cudnn-frontend so we should be able to remove some of the code (and reduce some maintenance cost).
| # TODO(KshitijLakhani): cuDNN FE can model bias input separately from dBias, | ||
| # but TE does not yet plumb whether dBias is requested into the common backend selector. | ||
| # Until that distinction is available, the D=256 SM10x gate requires no bias. |
There was a problem hiding this comment.
#2964 will plumb bias/dbias shapes/requirements to the C side, so hopefully when that's merged, this wouldn't be an issue.
Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
da25221 to
e3032d3
Compare
for more information, see https://pre-commit.ci
Signed-off-by: Kshitij Lakhani <klakhani@nvidia.com>
078c80f to
9921b99
Compare
for more information, see https://pre-commit.ci
Description
Support for D=256 BWD for Blackwell CC 10x via the C++ API (which TE uses) was added in cuDNN 9.23 + cuDNN FE 1.24 (BSHD) and cuDNN 9.30 + cuDNN FE 1.26 (THD)
Enabling this support in TE attention and adding conditional tests for the same
Type of change
Changes
Add guard when picking the backend (sub backend) in TE common.
Add tests for D=256 case in TE PyT and TE JAX
Checklist: