Repository navigation
Vulkan: support attn_mask and scale in llama.custom_sdpa / sdpa_with_kv_cache - #23539
msluszniak wants to merge 13 commits into
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/23539
Note: Links to docs will display an error until the docs builds have been completed.
|
|
@msluszniak this revert would have resolved the failing CI. Could you try with this and ping to trigger CI workflows? |
SS-JIA
left a comment
There was a problem hiding this comment.
🤖 AI review (Claude) on behalf of @SS-JIA.
The masked update_cache → custom_sdpa path is numerically correct on Adreno 750 and Mali-G715. That covers prefill and decode, texture and buffer, fp32 and fp16, GQA, an unaligned C, and C − S checked with a random finite mask. The causal path is unchanged.
Requested before landing:
- A test that actually covers
C − S. - A description update: Gemma 4 still can't reach this path, and masked decode costs O(C_max).
The load-time aborts (fp32 mask with fp16 q, constant mask) and the Low items could be follow-ups, since masked models aborted before this PR anyway. Details inline.
| const at::Tensor q_pos = at::arange(seq_len).unsqueeze(1) + input_pos; | ||
| const at::Tensor k_pos = at::arange(max_context_len).unsqueeze(0); | ||
| const at::Tensor visible = | ||
| at::logical_and(k_pos <= q_pos, k_pos > q_pos - sliding_window); |
There was a problem hiding this comment.
🤖 This comment was generated by an AI reviewer (Claude) on behalf of @SS-JIA. Please double-check the reasoning.
[Medium] The new tests can't detect a broken C − S substitution.
This sliding-window mask sets every key after the query's position to −inf. So the output is identical whether the shaders attend to all C keys (this PR) or only the first input_pos + S (the old behaviour): the extra columns contribute exp(−inf) = 0 either way. A regression that dropped the attn_input_pos substitution would still pass every new case.
I checked the mechanism on device with a random finite mask (torch.randn(S, C) * 2). With that mask, unwritten cache slots still add exp(0·…) to the softmax denominator, so the two behaviours differ. It matches eager:
- fp32: ≤ 3e-7;
- fp16: ≤ 9.8e-4;
- for both prefill (S=7) and decode (S=1, pos=12).
So the code is right, but CI doesn't cover it. Suggest adding one case with a non-causal or random-finite mask, for example a bidirectional prefix or random values.
Verified with an Android executor_runner built from this PR's head (EXECUTORCH_BUILD_VULKAN=ON) on a Galaxy S24 (Adreno 750). The graph is llama::update_cache → llama::custom_sdpa, with KV caches as mutable buffers, H=8/Hkv=2, D=64, C=30, scale=0.25, dynamic S≤8. It was exported with this PR's VulkanPartitioner and compared against an fp32 eager reference that applies the mask over all C keys.
There was a problem hiding this comment.
Added test_sdpa_op_random_mask_attends_full_cache in 6b399fe. It rewinds input_pos under a random finite mask, so dropping C - S fails it.
| // As in the CPU op, an attn_mask replaces the causal mask: it is a 2D | ||
| // [S, C] additive mask over every key in k_cache, and cannot be combined | ||
| // with is_causal. Without one, the attention is assumed to be causal. | ||
| const bool has_mask = graph.val_is_not_none(attn_mask); |
There was a problem hiding this comment.
🤖 This comment was generated by an AI reviewer (Claude) on behalf of @SS-JIA. Please double-check the reasoning.
[Medium] Gemma 4, the stated motivation, still can't reach this path, so please adjust the description.
custom_sdpa only reaches sdpa_with_kv_cache_impl through the causal_sdpa fusion in backends/vulkan/patterns/sdpa.py. That detector matches every custom_sdpa, but replace_custom_sdpa_with_causal_sdpa then asserts that llama::update_cache nodes feed the K/V caches. Gemma 4 (examples/models/gemma4/text_decoder/gemma4_attention.py) writes its caches with narrow().copy_() / index_copy_ instead, so a Vulkan export fails inside FusePatternsPass before this code runs.
I reproduced this: lowering a masked custom_sdpa that isn't fed by update_cache raises AssertionError at patterns/sdpa.py: replace_custom_sdpa_with_causal_sdpa.
This predates the PR, but the description says this enables Gemma 4 to delegate, and it doesn't yet. Options:
- make the detector return no match when
update_cacheis absent, so a plaincustom_sdpagoes throughsdpa_impl. TheC − Ssymint is already re-read on resize, so a narrowed K should work; - or narrow the description to
update_cache-based models.
Verified with an Android executor_runner built from this PR's head (EXECUTORCH_BUILD_VULKAN=ON) on a Galaxy S24 (Adreno 750). The graph is llama::update_cache → llama::custom_sdpa, with KV caches as mutable buffers, H=8/Hkv=2, D=64, C=30, scale=0.25, dynamic S≤8. It was exported with this PR's VulkanPartitioner and compared against an fp32 eager reference that applies the mask over all C keys.
There was a problem hiding this comment.
Right, Gemma 4 writes the cache with index_copy_ and can't reach this yet. Updated the description.
| VK_CHECK_COND( | ||
| graph.size_at<int64_t>(-1, attn_mask) == | ||
| graph.size_at<int64_t>(-3, k_cache)); | ||
| VK_CHECK_COND(graph.dtype_of(attn_mask) == graph.dtype_of(q_projected)); |
There was a problem hiding this comment.
🤖 This comment was generated by an AI reviewer (Claude) on behalf of @SS-JIA. Please double-check the reasoning.
[Medium] Mask configurations rejected here are still delegated, so they abort at load time instead of falling back to CPU.
register_sdpa_with_kv_cache / register_sdpa_cpp_ops in op_registry.py have no node check, so these new VK_CHECK_CONDs only fire at load. The most realistic trigger is this dtype check. The CPU op requires a Float mask (op_sdpa.cpp: "Attention mask must be a Float tensor"), so an fp16 model that is valid on CPU has fp16 q and an fp32 mask. On device, that model is delegated and then aborts:
Exception raised from sdpa_impl at .../SDPA.cpp:749:
(graph.dtype_of(attn_mask) == graph.dtype_of(q_projected)) is false!
The same applies to attn_mask together with is_causal=True, and to a non-2D mask.
It's not a regression, since every masked model aborted before this PR. But a check_node_fn mirroring these checks (dtype, 2D, is_causal, shape) would turn the aborts into CPU fallback. Alternatively, accept an fp32 mask with fp16 q by upcasting in the shader: the mask is already read as float in apply_attn_mask.
Verified with an Android executor_runner built from this PR's head (EXECUTORCH_BUILD_VULKAN=ON) on a Galaxy S24 (Adreno 750). The graph is llama::update_cache → llama::custom_sdpa, with KV caches as mutable buffers, H=8/Hkv=2, D=64, C=30, scale=0.25, dynamic S≤8. It was exported with this PR's VulkanPartitioner and compared against an fp32 eager reference that applies the mask over all C keys.
There was a problem hiding this comment.
Fixed in 71ef295. The partitioner rejects these configs, and so does the causal_sdpa detector, since its matched nodes skip per-op checks.
There was a problem hiding this comment.
AI review (Codex), static analysis.
[Medium] Nonblocking follow-up for an inherited fallback gap: these checks still skip auto_functionalized and auto_functionalized_v2 wrapping llama.sdpa_with_kv_cache. The partitioner resolves args[0] for registration but passes the wrapper here; node_has_target checks only the wrapper target, so arg_idx remains None and the predicate returns True. A CPU-valid Half-QKV/Float-[S,C]-mask call (is_causal=False, force_fp16=False, full fp16 buffer support) therefore delegates, then fails the runtime dtype check during model load after preprocessing restores the fused call. The same caller already failed before this PR; the direct-node fallback fix in this thread leaves the wrapped form unchanged. Consider resolving the underlying overload and reading normalized schema kwargs, including v2 cache base/view metadata, with real wrapped fused-SDPA coverage.
There was a problem hiding this comment.
Fixed in cbd09cf: the predicate now unwraps auto_functionalized(_v2) and reads schema kwargs, including the v2 key_cache base/view.
| VK_CHECK_COND(graph.dtype_of(attn_mask) == graph.dtype_of(q_projected)); | ||
| VK_CHECK_COND( | ||
| graph.storage_type_of(attn_mask) == graph.storage_type_of(q_projected)); | ||
| VK_CHECK_COND(graph.packed_dim_of(attn_mask) == WHCN::kWidthDim); |
There was a problem hiding this comment.
🤖 This comment was generated by an AI reviewer (Claude) on behalf of @SS-JIA. Please double-check the reasoning.
[Medium] A constant mask arrives as a TensorRef and is never prepacked, so load aborts.
After fusion, sdpa_with_kv_cache is registered with supports_prepacking=True, so insert_prepack_nodes does not add a prepack node for a constant attn_mask, such as a precomputed mask stored as a buffer. The storage/layout checks here, and the binding in sdpa_impl, then run on a TensorRef. On device:
Exception raised from toConstTensor at .../containers/Value.h:324: (isTensor()) is false!
Suggest prepack_standard(...) on the mask when graph.val_is_tref(attn_mask), using q's storage and width packing, or at least a clear VK_CHECK_COND(graph.val_is_tensor(attn_mask), ...) plus partitioner gating.
[Low] Related checks:
apply_attn_maskhard-codes a contiguousmask_Crow stride (buffer) and the standard axis map (texture), but onlypacked_dim == Wis checked. Consideris_contiguous/has_standard_axis_mapchecks.- The
[S, C]shape is only validated against the build-time max shapes. A runtime mismatch after resize silently skips rows or columns. - Most of the new
VK_CHECK_CONDs have no message, which makes these load failures hard to diagnose.
Verified with an Android executor_runner built from this PR's head (EXECUTORCH_BUILD_VULKAN=ON) on a Galaxy S24 (Adreno 750). The graph is llama::update_cache → llama::custom_sdpa, with KV caches as mutable buffers, H=8/Hkv=2, D=64, C=30, scale=0.25, dynamic S≤8. It was exported with this PR's VulkanPartitioner and compared against an fp32 eager reference that applies the mask over all C keys.
There was a problem hiding this comment.
Fixed in bd9b349, a constant mask is now prepacked.
There was a problem hiding this comment.
AI review (Codex), static analysis.
[Medium] The constant-mask prepack fix in this thread still compares the serialized dtype before resolving the mask. An all-fp32 update_cache → custom_sdpa model (B=1,S=4,C=8,Hq=2,Hkv=1,D=8) with a constant Float [S,C] mask and is_causal=False is admitted and fused to sdpa_with_kv_cache, leaving the mask as a TensorRef. With force_fp16=True, query metadata and mask bytes serialize as Half. On a device lacking either 16-bit storage buffers or shaderFloat16, query/cache vTensors become Float, including for texture storage, while the mask TensorRef remains Half. This check then fails during model load before reaching prepack_standard. That existing prepack/staging path supports the fallback and would produce a Float mask matching the query. Please compare graph.dtype_of(mask) after resolving the TensorRef, retain the shape/storage/layout checks, and cover a constant mask through normal fusion with fp16 buffer capabilities disabled.
There was a problem hiding this comment.
Fixed in 6928724: dtype is compared after prepacking, and added a constant-mask op test.
| // negative infinity. | ||
| #ifdef HAS_MASK | ||
| // The additive attn_mask replaces the causal mask, so no tile can be skipped. | ||
| const bool tile_in_mask_region = false; |
There was a problem hiding this comment.
🤖 This comment was generated by an AI reviewer (Claude) on behalf of @SS-JIA. Please double-check the reasoning.
[Medium, documentation] Masked decode always processes the full static cache.
With a mask, no tile can be skipped. After fusion, k_cache is the full C_max cache, so QK, softmax and AV all scale with the maximum context instead of input_pos + S. For example, a 512-token sliding window on a 4096-token cache does about 8× the QK/AV work of the causal path at every decode step.
This matches CPU semantics, so it isn't a correctness issue. It's a sizeable, non-obvious cost, though, and worth stating in the PR description. A longer-term option is supporting a narrowed K (see the comment on the description) or letting callers skip fully masked tiles.
There was a problem hiding this comment.
Noted in the description and in a comment in sdpa_impl.
| // kDummyValueRef to indicate no bias. scale_val is always passed as | ||
| // a spec const; the LLM path computes it per head_dim and FUSED may | ||
| // inherit from the caller-supplied scale. | ||
| // a spec const; both modes default it to 1/sqrt(head_dim) and use the |
There was a problem hiding this comment.
🤖 This comment was generated by an AI reviewer (Claude) on behalf of @SS-JIA. Please double-check the reasoning.
[Low] The updated comments don't match every caller.
This now says both modes "use the caller-supplied scale when there is one". But compute_attn_weight_with_kv_cache_impl (LLM mode) still (void)s both scale and attn_mask. Either honor the scale there and VK_CHECK_COND(val_is_none(attn_mask)), or scope the comment.
The "LLM: pass input_pos_symint (real symint)" line above is also inaccurate in the masked case, where the C − S symint is passed instead.
There was a problem hiding this comment.
Fixed in e865d46. The test op now honors scale and rejects a mask.
| #ifdef INPUT_BUFFER | ||
| const float mask_val = float(t_mask[s_idx * mask_C + c_idx]); | ||
| #else | ||
| const float mask_val = float(texelFetch( |
There was a problem hiding this comment.
🤖 This comment was generated by an AI reviewer (Claude) on behalf of @SS-JIA. Please double-check the reasoning.
[Low] The texture path fetches each mask texel 4 times.
texelFetch runs once per element, so the 4 components of a texel are fetched 4 times. Texture rows are padded to texels, so the texture branch can fetch once per c4 and index the components. Only the buffer branch needs scalar reads, because its rows aren't texel-aligned when C % 4 != 0. This is a small cost, but it sits in the QK epilogue on every tile.
| } | ||
| } | ||
|
|
||
| TEST(VulkanSDPATest, test_sdpa_op_sliding_window_mask) { |
There was a problem hiding this comment.
🤖 This comment was generated by an AI reviewer (Claude) on behalf of @SS-JIA. Please double-check the reasoning.
[Low] Other test gaps (in addition to the non-causal mask case above):
- No MHA case: both configs (8/2, 6/2) are GQA, so the non-GQA decode AV shader isn't exercised with
C − S. - Scale/mask combinations: no explicit
scalewithout a mask (this PR also enables a custom scale on the causal path), and no mask withscale=None. - Entry points: only
sdpa_with_kv_cache.defaultis called. There's no directcustom_sdpatest and no export/partitioner test (e.g.test_vulkan_delegate.py) covering the fusion with a mask. - Head dims: only multiples of 4.
For reference, these pass on device with this head: a sliding-window mask on texture and buffer, fp32/fp16, prefill (S=7/3/8) and decode, plus the unmasked causal path. Max error vs eager is ≤ 2.4e-7 (fp32) and ≤ 9.8e-4 (fp16) on both Adreno 750 and Mali-G715.
There was a problem hiding this comment.
Added MHA, scale/mask combinations and a direct custom_sdpa test in 6b399fe. Head dims not a multiple of 4 aren't covered on the causal path either, so I left that out.
SS-JIA
left a comment
There was a problem hiding this comment.
Some seemingly valid comments from AI review
…kv_cache The LLM SDPA path rejected both arguments at load. Sliding window attention (e.g. Gemma) needs them: an additive mask in place of the causal one, and a scale other than 1/sqrt(head_dim). As in the CPU op, the mask is a 2D [S, C] tensor over every key in the cache and cannot be combined with is_causal. The QK shaders get _mask variants that add it instead of applying the causal mask; softmax and AV are given C - S in place of input_pos so they attend to all C keys, which needs no shader change.
…test op The QK node comment claimed both modes use the caller-supplied scale, but the compute_attn_weight_with_kv_cache test op ignored it, and the LLM input_pos note did not cover the masked C - S case. The test op now uses scale and rejects an attn_mask it would otherwise silently drop.
… resize A constant mask reaches the fused sdpa_with_kv_cache as a TensorRef, since the op handles its own prepacking, and was then used as a tensor. Prepack it with q's storage and width packing. Also check that the mask is contiguous (buffer) or uses the standard axis map (texture), as the shaders assume, that it stays [S, C] after a resize, and give the mask checks messages.
The runtime rejects an attn_mask combined with is_causal, a mask that is not 2D [S, C] or not of q's dtype (e.g. the fp32 mask the CPU op requires next to an fp16 query), and non-causal attention without a mask. These were still delegated and aborted at load. Check them in the partitioner, and in the causal_sdpa pattern detector, since a matched node skips the per-op checks. The update_cache nodes that write the cache of a rejected custom_sdpa stay on CPU with it; splitting them fails at lowering.
apply_attn_mask fetched a texture mask texel once per element. Texture rows are padded to texels, so fetch once per 4 columns; the buffer path still reads element by element since its rows are not texel aligned.
A sliding window mask is -inf after each query's position, so it cannot tell whether the shaders attend to all C keys or only to input_pos + S. Add a random finite mask test where the cache is filled first and input_pos is then rewound, which only passes if every key is attended. Also cover MHA, a mask without a scale, an explicit scale on the causal path, and the unfused update_cache + llama.custom_sdpa entry point.
b0c6f38 to
6b399fe
Compare
|
|
||
| import torch | ||
|
|
||
| from executorch.backends.vulkan.op_registry import check_llm_sdpa_node |
There was a problem hiding this comment.
🤖 This comment was generated by an AI reviewer (Claude) on behalf of @SS-JIA. Please double-check the reasoning.
[High] This import creates a circular import between op_registry and patterns.
op_registry -> custom_ops_lib -> patterns/__init__ -> patterns/sdpa -> op_registry (partially initialized)
In OSS this is masked: backends/vulkan/__init__.py loads vulkan_partitioner, which imports patterns before anything imports op_registry. Buck targets don't include that __init__, though, so any target or entry point that imports op_registry first breaks. backends/webgpu/partitioner/partitioner.py:12 (from executorch.backends.vulkan.op_registry import OpKey) does exactly that.
Reproduced by loading op_registry first without running the package __init__, i.e. what a Buck target sees:
import sys, types, importlib
import executorch.backends
pkg = types.ModuleType("executorch.backends.vulkan")
pkg.__path__ = ["<repo>/backends/vulkan"]
sys.modules["executorch.backends.vulkan"] = pkg
importlib.import_module("executorch.backends.vulkan.op_registry")ImportError: cannot import name 'check_llm_sdpa_node' from partially initialized module
'executorch.backends.vulkan.op_registry' (most likely due to a circular import)
The same snippet imports cleanly on main.
It can't be fixed by adding a Buck dependency either: vulkan_patterns → :op_registry would close the cycle op_registry → custom_ops_lib → vulkan_patterns.
Suggested fix: move check_llm_sdpa_node and its helpers into a module that only depends on utils, either patterns/sdpa.py itself or a small new sdpa_checks.py. Then have op_registry import the check from there, so the dependency only points from op_registry to patterns.
Everything else from the previous round looks addressed. With this head on Adreno 750 and Mali-G715, all masked/causal/random-mask/constant-mask cases match eager, and an fp32 mask with fp16 q now stays on CPU. So this is the one blocker I see.
There was a problem hiding this comment.
Fixed in 05bf67f: check_llm_sdpa_node now lives in patterns/sdpa.py, which no longer imports op_registry, so the cycle is gone.
Breaks the op_registry -> custom_ops_lib -> patterns -> op_registry import cycle; op_registry now imports the check from patterns.
On a device without full fp16 buffer support a Half q falls back to Float, and prepacking the mask TensorRef applies the same fallback, so the serialized Half dtype must not be compared before it. Add an op test for a constant mask through both entry points.
The partitioner passes the auto_functionalized wrapper to check_llm_sdpa_node, which then saw no LLM SDPA target and admitted every mask configuration. Resolve the wrapped op and read its schema kwargs, including the key_cache base and view of auto_functionalized_v2.
torch.export drops trailing arguments that equal their defaults, so a masked custom_sdpa with is_causal=False and no scale has five args and CausalSDPAMatch raised IndexError during partitioning.
|
@pytorchbot label "release notes: vulkan" |
…-mask # Conflicts: # backends/vulkan/test/test_vulkan_passes.py
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* feat: export LFM3-1.7B and LFM3-VL-2B for XNNPACK, MLX and Vulkan Vendored LFM3 decoder (sliding-window + gated attention, zero-tail partial RoPE, sandwich norm), exact vs the upstream remote code in fp32. VL adds a fixed 512x512 vision tower, exact vs get_image_features. Context 8192. Vulkan lowers only once pytorch/executorch#23539 is on the fork; see NOTES. * feat: export LFM3-Audio-2B (audio in, text out); quantize LFM3 layer by layer Audio encoder with the log-mel front end in the graph (DFT conv, no stft), matching upstream get_audio_features; text decoder masks the speech-mode tokens. Quantizing one fp32 layer at a time keeps export RAM near the bf16 model size with an identical result. * lfm3_audio: expose get_audio_frames_per_token * lfm3_audio: fix XNNPACK lowering and assets, bmm attention, results * lfm3_audio: MLX fixes (fp32 log-mel, in-graph RoPE), results * lfm3: drop Vulkan (slower than XNNPACK on device)
Summary
The Vulkan LLM SDPA path (
llama.custom_sdpa/sdpa_with_kv_cache) rejectedattn_maskandscaleat load, so masked attention could not be delegated for models that write the cache withllama::update_cache(e.g.examples/models/llamawithuse_attention_mask). Gemma 4 writes it withindex_copy_and still cannot reach this path. This adds them with the CPU op's semantics: a 2D[S, C]additive mask over every key in the cache, not combinable withis_causal.The QK shaders get
_maskvariants. Softmax and AV are unchanged: they are givenC - Sin place ofinput_pos, so they attend to allCkeys. With a mask, decode therefore costs O(C_max).Test plan
test_sdpa_op_sliding_window_mask*cases insdpa_test.cpp(texture/buffer, fp32/fp16, a context length that is not a multiple of 4).test_sdpa_op_random_mask_attends_full_cache(fails withoutC - S), MHA, scale/mask combinations, and partitioner tests for configs that must stay on CPU.update_cache+custom_sdpawith a sliding-window mask andscale=1.0, dynamic seq len, prefill and decode at 5 shapes, vs eager (before the review changes): max abs diff 2.7e-6 (fp32) and 6.2e-3 (fp16) on texture and on buffer with C=62. The causal path is unchanged.cc @SS-JIA @manuelcandales @digantdesai @cbilgin @nil-is-all