-
Notifications
You must be signed in to change notification settings - Fork 778
[JAX] [PyT] [Common] Enable D=256 BWD cuDNN fused attn for Blackwell CC 10.x #3056
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
f108426
8ca68e2
a093127
b7dc18c
28fe7ba
dd6cbf3
9920cbe
3f06289
38de70a
021c450
b597eda
4882cda
4d5ad94
6184e85
424b2c4
fe7c6b2
cfed53f
5767db0
3035301
06eee35
ee79662
cbab884
00d14cd
ed4b745
e3032d3
6f2b748
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -469,7 +469,7 @@ def _get_max_segments_per_sequence(self): | |
| return 1 | ||
|
|
||
| def _check_configs(self): | ||
| # TODO(rewang): probably adds this in is_fused_attn_available | ||
| # TODO(KshitijLakhani): probably add/move this to is_fused_attn_available | ||
| if self.qkv_layout.is_thd() and not self.attn_mask_type.is_padding(): | ||
| pytest.skip("THD format requires padding masks.") | ||
|
|
||
|
|
@@ -493,11 +493,62 @@ def _check_configs(self): | |
| pytest.skip( | ||
| "seqlen_q > seqlen_kv is not supported with sliding window attention in cuDNN" | ||
| ) | ||
| compute_capability = get_device_compute_capability(0) | ||
| cudnn_version = get_cudnn_version() | ||
| # D=256 bprop on SM10x uses the deterministic algorithm path only. BSHD support | ||
| # starts with cuDNN FE 1.24 / BE 9.23; THD execution-plan support starts with | ||
| # cuDNN FE 1.26 / BE 9.25. 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): | ||
| if self.head_dim_qk != 256 or self.head_dim_v != 256: | ||
| pytest.skip( | ||
| "D=256 BWD on Blackwell only supports d_qk == d_v == 256;" | ||
| f" got d_qk={self.head_dim_qk}, d_v={self.head_dim_v}." | ||
| ) | ||
| required_cudnn_version = 92500 if self.qkv_layout.is_thd() else 92300 | ||
| required_cudnn_version_label = "9.25" if self.qkv_layout.is_thd() else "9.23" | ||
| if cudnn_version < required_cudnn_version: | ||
| pytest.skip( | ||
| f"D=256 BWD on Blackwell with {self.qkv_layout} requires cuDNN" | ||
| f" {required_cudnn_version_label} or newer; got cuDNN {cudnn_version}." | ||
| ) | ||
| # 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. | ||
|
Comment on lines
+516
to
+518
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've opened a GH issue for this: #3188
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #2964 will plumb bias/dbias shapes/requirements to the C side, so hopefully when that's merged, this wouldn't be an issue. |
||
| unsupported = None | ||
| if self.attn_bias_type == AttnBiasType.PRE_SCALE_BIAS: | ||
| unsupported = "pre-scale bias" | ||
| elif self.attn_bias_type != AttnBiasType.NO_BIAS: | ||
| unsupported = ( | ||
| "post-scale bias in TE's D=256 backend gate; bias-input-only" | ||
| " support needs TE to distinguish between bias input and dBias" | ||
| ) | ||
| elif self.dropout_prob != 0.0: | ||
| unsupported = "dropout" | ||
| elif self.softmax_type != AttnSoftmaxType.VANILLA_SOFTMAX: | ||
| unsupported = "non-vanilla softmax" | ||
| if unsupported is not None: | ||
| pytest.skip( | ||
| "D=256 BWD on Blackwell uses the deterministic SM100 D=256 SDPA BWD" | ||
| f" kernel which does not support {unsupported}." | ||
| ) | ||
| if self.window_size is not None and self.window_size != (-1, -1): | ||
| if not self.attn_mask_type.is_causal(): | ||
| pytest.skip( | ||
| "D=256 BWD on Blackwell uses the SM100 D=256 SDPA BWD kernel" | ||
| " which requires window_size=(-1, -1) for non-causal masks." | ||
| ) | ||
| if self.window_size[1] not in (-1, 0): | ||
| pytest.skip( | ||
| "D=256 BWD on Blackwell only supports right window -1 or 0" | ||
| " for causal masks." | ||
| ) | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aren't these checks duplicate to the checks we added on the C++ side? Would the call
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So if we are just interested in the gating effect, you are right. The 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 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() |
||
| if get_device_compute_capability(0) >= 100 and self.is_training: | ||
| if compute_capability >= 100 and self.is_training: | ||
| if FusedAttnHelper.is_non_deterministic_allowed() and ( | ||
| (self.dropout_prob != 0.0 and self.attn_bias_type != AttnBiasType.NO_BIAS) | ||
| or get_cudnn_version() < 90700 | ||
| or cudnn_version < 90700 | ||
| ): | ||
| pytest.skip( | ||
| "For sm100+, non-deterministic bprop (cuDNN 9.7+) does not support bias with" | ||
|
|
@@ -506,7 +557,7 @@ def _check_configs(self): | |
| if not FusedAttnHelper.is_non_deterministic_allowed() and ( | ||
| self.dropout_prob != 0.0 | ||
| or self.attn_bias_type != AttnBiasType.NO_BIAS | ||
| or get_cudnn_version() < 91801 | ||
| or cudnn_version < 91801 | ||
| ): | ||
| pytest.skip( | ||
| "For sm100+, deterministic bprop (cuDNN 9.18.1+) does not support bias or" | ||
|
|
@@ -1642,6 +1693,32 @@ def test_backward( | |
| QKVLayout.THD_THD_THD, | ||
| id="2-1024-2048-12-6-128-64-BF16-CROSS-GQA-RAGGED_SEPARATE", | ||
| ), | ||
| # D=256 deterministic backward on the SM100 dedicated SDPA bprop kernel. | ||
| # BSHD requires cuDNN FE 1.24 / BE 9.23+; THD requires cuDNN FE 1.26 / BE 9.25+. | ||
| pytest.param( | ||
| 4, | ||
| 128, | ||
| 128, | ||
| 16, | ||
| 16, | ||
| 256, | ||
| 256, | ||
| jnp.float16, | ||
| QKVLayout.BSHD_BS2HD, | ||
| id="4-128-128-16-16-256-256-FP16-SELF-KV_PACKED", | ||
| ), | ||
| pytest.param( | ||
| 4, | ||
| 128, | ||
| 128, | ||
| 16, | ||
| 16, | ||
| 256, | ||
| 256, | ||
| jnp.float16, | ||
| QKVLayout.THD_T2HD, | ||
| id="4-128-128-16-16-256-256-FP16-SELF-RAGGED_KV_PACKED", | ||
| ), | ||
| ], | ||
| ) | ||
| @pytest.mark.parametrize( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these checks already be covered on the C side? Are these by any chance duplicate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).