Skip to content

Commit 08bbc41

Browse files
feich-msclaude
andcommitted
Address review: fix dummy-K buffer size and add const_cast safety guard
- Size kDummy/kRotary with sequence_length (not 1) to match the kernel's iteration domain, preventing OOB writes during prefill (q_seq > 1) - Add ORT_ENFORCE in flash_attention kv_empty path to guard against future refactors that might write through the const_cast'd pointers - Add new test SharedKV_EmptyKV_WithPast_Rotary_Prompt_WebGPU exercising the rotary + kv_empty path with q_seq_len=6 Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
1 parent 27412d4 commit 08bbc41

3 files changed

Lines changed: 39 additions & 4 deletions

File tree

onnxruntime/contrib_ops/webgpu/bert/flash_attention.cc

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -471,12 +471,14 @@ Status ApplyFlashAttention(const Tensor* Q, const Tensor* K, const Tensor* V, co
471471
// Use past_key/past_value directly as the present buffers for attention.
472472
ORT_ENFORCE(!do_rotary, "Fused SplitPackedQKVWithRotaryEmbeddingAndCopyKV should not be used with kv_sequence_length==0.");
473473
if (past_key != nullptr && past_value != nullptr) {
474-
// Safe: flash attention kernels only read from present_key/present_value.
475-
// CopyKVCache is skipped when kv_empty, so no writes through these pointers.
474+
// Alias past as present — flash attention only reads present_key/present_value,
475+
// and CopyKVCache is skipped when kv_empty, so no writes occur through these pointers.
476476
present_key = const_cast<Tensor*>(past_key);
477477
present_value = const_cast<Tensor*>(past_value);
478478
}
479479
// If past is also null, present_key/present_value were already set to internal empty tensors above.
480+
ORT_ENFORCE(!parameters.past_present_share_buffer_,
481+
"kv_empty path must not use past_present_share_buffer (CopyKVCache is skipped).");
480482
} else if (do_rotary) {
481483
ORT_ENFORCE(parameters.is_packed_qkv_, "Fused SplitPackedQKVWithRotaryEmbeddingAndCopyKV requires packed QKV input.");
482484
ORT_ENFORCE(parameters.past_present_share_buffer_, "Fused SplitPackedQKVWithRotaryEmbeddingAndCopyKV requires static KV cache.");

onnxruntime/contrib_ops/webgpu/bert/group_query_attention.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -292,8 +292,8 @@ Status GroupQueryAttention::ComputeInternal(onnxruntime::webgpu::ComputeContext&
292292
// Apply RoPE to Q only. Use the fused kernel with a dummy K to avoid zero-element buffers.
293293
// K output is discarded since attention uses past_key/past_value.
294294
qRotary = context.CreateGPUTensor(query->DataType(), query->Shape());
295-
kDummy = context.CreateGPUTensor(query->DataType(), TensorShape({parameters.batch_size_, 1, parameters.kv_hidden_size_}));
296-
kRotary = context.CreateGPUTensor(query->DataType(), TensorShape({parameters.batch_size_, 1, parameters.kv_hidden_size_}));
295+
kDummy = context.CreateGPUTensor(query->DataType(), TensorShape({parameters.batch_size_, parameters.sequence_length_, parameters.kv_hidden_size_}));
296+
kRotary = context.CreateGPUTensor(query->DataType(), TensorShape({parameters.batch_size_, parameters.sequence_length_, parameters.kv_hidden_size_}));
297297
ORT_RETURN_IF_ERROR(RunFusedQKRotaryEmbedding(context, parameters,
298298
query, &kDummy,
299299
seqlen_k,

onnxruntime/test/contrib_ops/group_query_attention_op_test.cc

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1884,5 +1884,38 @@ TEST(GroupQueryAttentionTest, SharedKV_EmptyKV_WithPast_Rotary_WebGPU) {
18841884
ExpectOutputsMatch(webgpu_output, cpu_output, 0.05f, "SharedKV_Rotary_WebGPU_vs_CPU");
18851885
}
18861886

1887+
// WebGPU: shared KV with do_rotary=1 and q_seq_len > 1 (prefill phase).
1888+
// Validates that the dummy-K rotary buffer is correctly sized to sequence_length.
1889+
TEST(GroupQueryAttentionTest, SharedKV_EmptyKV_WithPast_Rotary_Prompt_WebGPU) {
1890+
auto webgpu_ep = DefaultWebGpuExecutionProvider();
1891+
if (!webgpu_ep) {
1892+
GTEST_SKIP() << "WebGPU EP not available";
1893+
}
1894+
1895+
constexpr int batch_size = 1;
1896+
constexpr int q_seq_len = 6;
1897+
constexpr int past_seq_len = 8;
1898+
constexpr int num_heads = 2;
1899+
constexpr int kv_num_heads = 1;
1900+
constexpr int head_size = 16;
1901+
constexpr int hidden_size = num_heads * head_size;
1902+
1903+
std::vector<float> query_data(batch_size * q_seq_len * hidden_size);
1904+
std::vector<float> past_key_data(batch_size * kv_num_heads * past_seq_len * head_size);
1905+
std::vector<float> past_value_data(batch_size * kv_num_heads * past_seq_len * head_size);
1906+
for (size_t i = 0; i < query_data.size(); i++) query_data[i] = 0.1f * static_cast<float>(i % 11 + 1);
1907+
for (size_t i = 0; i < past_key_data.size(); i++) past_key_data[i] = 0.2f * static_cast<float>(i % 5 + 1);
1908+
for (size_t i = 0; i < past_value_data.size(); i++) past_value_data[i] = 0.3f * static_cast<float>(i % 3 + 1);
1909+
1910+
auto webgpu_output = RunGQASharedKVWithRotary(
1911+
batch_size, q_seq_len, past_seq_len, query_data, past_key_data, past_value_data,
1912+
num_heads, kv_num_heads, head_size, /*use_cuda=*/false, /*use_webgpu=*/true);
1913+
auto cpu_output = RunGQASharedKVWithRotary(
1914+
batch_size, q_seq_len, past_seq_len, query_data, past_key_data, past_value_data,
1915+
num_heads, kv_num_heads, head_size, /*use_cuda=*/false, /*use_webgpu=*/false);
1916+
1917+
ExpectOutputsMatch(webgpu_output, cpu_output, 0.05f, "SharedKV_Rotary_Prompt_WebGPU_vs_CPU");
1918+
}
1919+
18871920
} // namespace test
18881921
} // namespace onnxruntime

0 commit comments

Comments
 (0)