Fix CPU Attention softcap/attn_mask ordering (onnx#7867, #7913) + consolidate CUDA spec coverage tests from #28371

[titaiwangms]

Fix CPU Attention softcap/attn_mask ordering to match ONNX v24 spec (#7867, #7913) + consolidate CUDA spec coverage tests (#28371)

Branch: fix-cpu-attention-softcap-mask-ordering (off microsoft/onnxruntime main)
Tip: bfe33d3d08

Supersedes #28370 and #28371.

• Fix softcap/attn_mask ordering in CUDA Attention to match ONNX v24 spec and CPU EP #28370 diagnosed the symptom correctly but inverted the fix: it modified the (already spec-correct) CUDA path to match the (buggy) CPU path. This PR takes the opposite — and correct — approach, aligning CPU with both CUDA and the ONNX v24 spec as established by Fix Attention op softcap ordering: apply before mask/bias onnx/onnx#7867 and Swap qk_matmul_output_mode 1 & 2 to match computation order onnx/onnx#7913.
• Add CUDA Attention spec-coverage tests (GQA asymmetric head-size, output_qk, fp32 softcap+mask ordering) #28371 ("Tighten CUDA Attention MEA eligibility on head_size%4 + add GQA test coverage") has been fully consolidated into this PR at the user's request. The CUDA test coverage and SKILL.md cleanup that lived on Add CUDA Attention spec-coverage tests (GQA asymmetric head-size, output_qk, fp32 softcap+mask ordering) #28371 share a qk_matmul_output_mode numbering and softcap/mask-ordering spec with the CPU fix here, so keeping them on separate PRs created merge-ordering and mental-model hazards. Rolling them up presents one coherent fix-tests-docs reviewer story.

We recommend closing both #28370 and #28371 in favor of this one.

---

Scope

1. CPU spec-correct softcap+mask ordering fix (per Fix Attention op softcap ordering: apply before mask/bias onnx/onnx#7867) — the headline bug. The CPU attention.cc was applying attn_mask before softcap (which would clip a -inf masked logit to a finite value, leaking the masked V slot through softmax). Now applies scale → softcap → +attn_mask → softmax in spec order.

2. qk_matmul_output_mode enum value swap (per Swap qk_matmul_output_mode 1 & 2 to match computation order onnx/onnx#7913). Breaking change to mode 1 ↔ 2 semantics + matching C++ enumerator rename. CPU op fully implements the new ordering; CUDA op (Flash/MEA/Unfused dispatch) confirmed unchanged in behaviour.

3. Tianlei blocker tests (CPU): mode-1+softcap differentiating, softcap+nonpad_kv_seqlen no-leakage. Both failed before this PR's fix.

4. Comprehensive CUDA Attention spec coverage (consolidated from Add CUDA Attention spec-coverage tests (GQA asymmetric head-size, output_qk, fp32 softcap+mask ordering) #28371):

   • TestONNXAttentionGQAAsymmetricHeadSize (REG) — guards the silent-broken-output regression on GQA + asymmetric Q/V head sizes (CUDA Attention kernel crashes with mismatched Q/K head dimensions (head_size != v_head_size) #28357, fixed by Fix CUDA Attention dispatch: skip MEA when head_size != v_head_size in GQA #28358).
   • TestONNXAttentionGQAOutputQK (1c) — pins GQA + raw-QK output (qk_matmul_output_mode == kQK) on the unfused fp16 path.
   • TestONNXAttentionGQASoftcapFloat32 (1e baseline) + TestONNXAttentionGQASoftcapFloat32MaskOrdering (1e ordering) — pins fp32 + softcap on the unfused GQA path, including the post-Clean up CPU kernel definition for opset 13 Pad #7867 softcap+mask ordering using a poison-V leakage detector.

5. SKILL.md updates (.agents/skills/cuda-attention-kernel-patterns/SKILL.md):

   • §4 (Softcap Ordering) revert — undoes Fix softcap/attn_mask ordering in CUDA Attention to match ONNX v24 spec and CPU EP #28370's inverted statement.
   • §1 (MEA eligibility paragraph) cleanup — head_size%8 is enforced by has_memory_efficient_attention; head_size == v_head_size is required for GQA (LaunchUngroup) in addition to decode (LaunchConcatNewToPastKV). Reflects the post-Fix CUDA Attention dispatch: skip MEA when head_size != v_head_size in GQA #28358 host-side gate.

6. ONNX node-test skip lists (transient, until ONNX v1.22+ is bundled):

   • C++ runner: onnxruntime/test/onnx/TestCase.cc::GetBrokenTests() — 20 attention entries.
   • Python runner: onnxruntime/test/testdata/onnx_backend_test_series_filters.jsonc::current_failing_tests — 7 CPU attention entries.

   Both block-comments cite Fix Attention op softcap ordering: apply before mask/bias onnx/onnx#7867 + Swap qk_matmul_output_mode 1 & 2 to match computation order onnx/onnx#7913 and the v1.22+ unblock criterion: the fixtures bundled in v1.21.0 were generated before the spec PRs landed and therefore encode the now-incorrect ordering. Once cmake/external/onnx is bumped to ≥1.22, both skip blocks should be removed in one commit.

---

Cross-references

• Issue: ONNX Attention CUDA: Remaining Opset 24 Feature Gaps #28351 (CPU spec coverage gaps), CUDA Attention kernel crashes with mismatched Q/K head dimensions (head_size != v_head_size) #28357 (asymmetric-V regression).
• Closed/replaced: Fix softcap/attn_mask ordering in CUDA Attention to match ONNX v24 spec and CPU EP #28370 (inverted fix), Add CUDA Attention spec-coverage tests (GQA asymmetric head-size, output_qk, fp32 softcap+mask ordering) #28371 (consolidated here).
• Coordinated with: Fix CUDA EP: opset 24 kernel registrations + CUTLASS alignment + MEA dispatch #28365 (qk_matmul_output_mode rename across consumers).
• Spec PRs: Fix Attention op softcap ordering: apply before mask/bias onnx/onnx#7867, Swap qk_matmul_output_mode 1 & 2 to match computation order onnx/onnx#7913 (supersedes the now-closed Softcap in Attention op onnx/onnx#7865 issue).

---

Verification

• AttentionTest.* — 60/60 pass (gtest, CPU + CUDA).
• TestONNXAttentionCPUSoftcapMaskOrdering — 4/4 pass (CPU spec fix + Tianlei blockers).
• TestONNXAttentionGQAAsymmetricHeadSize — 2/2 pass (fp16 + bf16, H100).
• TestONNXAttentionGQAOutputQK — 1/1 pass (fp16, H100).
• TestONNXAttentionGQASoftcapFloat32 + …MaskOrdering — 4/4 pass (fp32, H100).
• ONNX node tests: previously-failing 20 (C++) + 7 (Python) attention fixtures now SKIPPED with citation, not failed.
• lintrunner clean on all touched files.

[Copilot]

Pull request overview

Aligns ONNX Attention handling with the corrected softcap→mask/bias ordering and updates the auxiliary qk_matmul_output_mode naming/mapping used by the CPU/CUDA Attention codepaths and tests.

Changes:

• Refactors the CPU Attention score pipeline so softcap is applied before additive mask/bias, with updated snapshot handling for output_qk.
• Swaps/renames qk_matmul_output_mode enum values 1 and 2 to match the corrected stage ordering.
• Adds/updates Python and C++ regression coverage plus related CUDA skill/docs comments.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
onnxruntime/test/python/transformers/test_onnx_attention/test_gqa.py	Adds CPU-side Python canaries for softcap/mask leakage behavior.
onnxruntime/test/providers/cpu/llm/attention_op_test.cc	Updates qk_matmul baselines and adds C++ poison-V regression tests.
onnxruntime/core/providers/cuda/llm/attention.cc	Renames accepted qk_matmul_output_mode enum references/messages on CUDA.
onnxruntime/core/providers/cpu/llm/attention.cc	Implements the CPU production fix for score ordering and output snapshot staging.
onnxruntime/core/providers/cpu/llm/attention_parameters.h	Renames/swaps QKMatMulOutputMode enum values to the corrected meanings.
.agents/skills/cuda-attention-kernel-patterns/SKILL.md	Updates CUDA Attention skill guidance to reflect the corrected ordering/history.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[titaiwangms]

Review — multi-model pass (Opus 4.7 lead, with GPT-5.3-Codex / GPT-5.5 / Sonnet 4.6 reviewers)

Edit (post-discussion with author). Finding #1 below was originally framed as “add opset-version gating to preserve as-shipped opset-23 semantics.” That framing was wrong: opset 23 shipped with the leakage bug in both spec text and reference impl, and onnx/onnx#7867 + #7913 are post-release errata. Preserving the as-shipped opset-23 behavior would mean knowingly keeping the leak. The author’s position — that ORT should honor the corrected ordering/numbering for both v23 and v24 ahead of the ONNX v1.22 spec text — is the right call. Finding #1 is therefore reframed as a documentation ask, not a behavior change. Finding #2 (no differentiating test) becomes more important under this stance, since the test suite is the de-facto contract pinning ORT’s chosen interpretation.

The CPU softcap-before-mask fix itself is the right call: poison-V tests are an excellent oracle, the fold_mask_into_gemm refactor is clean, and the deep-copy-before-mutate snapshot discipline avoids the obvious alias trap. Mask handling for -inf is now correct.

That said, the cross-family review surfaced two non-trivial concerns and a handful of polish items.

🔴 Major

1. Document that ORT intentionally leads the spec on qk_matmul_output_mode semantics for opset 23.

The CPU kernel registers separate v23 and v24 kernels (cpu/llm/attention.cc:25-49), and CUDA does the same (cuda/llm/attention.cc:40-69), but both versions instantiate the same Attention<T> whose constructor reads the integer with no info.node().SinceVersion() check. Per onnx/onnx#7913 the integers 1 and 2 were swapped between the as-shipped opset 23 and the corrected pipeline, so an opset-23 model carrying value 1 will resolve to kPostSoftCap instead of the as-shipped “post-mask/bias” meaning.

This is intentional and correct — opset 23 shipped with both the wrong ordering and the wrong numbering, and #7867/#7913 are post-release errata that ORT is implementing ahead of ONNX v1.22. But the comment in attention_parameters.h:14 currently just says “v23/24 use the new numbering,” which reads as a passive statement of fact and obscures that ORT is deliberately diverging from the as-shipped opset-23 spec text.

Suggested fix: expand the comment to make the intent explicit, e.g. “ORT honors the corrected #7867/#7913 ordering/numbering for both opset 23 and opset 24, ahead of the ONNX v1.22 spec text errata. Producers that emitted opset-23 nodes against the as-shipped numbering will see the swapped auxiliary output post-merge; the as-shipped behavior is the leakage bug being fixed.” That makes the policy choice deliberate rather than accidental, and gives downstream tooling a clear pointer.

2. The enum swap is not actually exercised by a differentiating test.

attention_op_test.cc:1544-1571 updates the QK-output test, but the mode-1 case runs with softcap = NaN (i.e. softcap disabled), where kPostSoftCap and kQK are observationally identical. The mode-2 case is also softcap-off, so it only verifies the no-softcap commutative case. The riskiest behavior change in this PR — that mode 1 returns softcap(scale*QK) and mode 2 returns softcap(scale*QK) + mask/bias — is not pinned by any test.

This is more important than it would be under a strict version-gated implementation: because ORT is intentionally leading the spec, the test suite is the contract that documents the chosen interpretation. Without a differentiating test, a future refactor could quietly invert the snapshots and CI would not notice.

Suggested fix: one C++ CPU test with softcap > 0, non-zero additive mask, output-3 wired, and explicit expected values for modes 1 and 2. Bonus: a third case with nonpad_kv_seqlen enabled to lock in that kPostMaskBias includes nonpad masking (currently a defensible-but-undocumented choice — see polish item below).

🟡 Minor

3. Naming inconsistency in QKMatMulOutputMode (attention_helper.h).
Three of the four values follow a kPost* "stage you exited" convention; kQK is the lone outlier and its name implies "Q times K" even though scaling has already been applied. Renaming to kPostGemm or kPreSoftCap would make the enum self-describing. (Acknowledged: the names mirror the spec text, so this may be intentional — flagging for consideration.)

4. snapshot_needs_pre_mask conflates two orthogonal conditions (attention.cc:~153). The expression gates on both out_qk != nullptr and the mode, but the name only describes the mode half. Splitting into a mode-only predicate (mode_requires_pre_mask_snapshot) and combining with the buffer-presence check at the use site would make the intent obvious. Not a correctness bug, but a trap for the next maintainer.

5. Silent perf regression on the softcap + mask path (attention.cc:504-526). With softcap enabled, the mask can no longer fold into the GEMM beta=1, so the path becomes GEMM(beta=0) → in-place softcap → separate AddInPlace over the full S×T scores per head. That is one extra read/write pass over the score matrix — measurable for long-context CPU attention. Not a regression to revert (correctness wins), but worth a benchmark and possibly a fused softcap+mask pass for follow-up.

6. New MLFloat16 AddInPlace fallback lacks focused coverage (attention.cc:111-115, 148-151). The scalar MLFloat16(scores[i].ToFloat() + addend[i].ToFloat()) path only fires off-ARM-NEON, and fp16 numerics are exactly where ordering changes are most likely to surface small parity drifts. The new Python ordering tests are fp32-only by design (per the comment at test_gqa.py:1931-1936), so the fp16 path on x86 has no targeted softcap+mask test.

🟢 Polish

7. kPostMaskBias snapshot is taken after the nonpad_kv_seqlen mask (attention.cc:540-543). Defensible — “post-mask/bias” arguably encompasses nonpad masking — but worth a one-line comment so the next reader doesn’t wonder.

8. "Pre-fix / post-fix" language in test_gqa.py class docstring and SKILL.md. Useful for the PR conversation, confusing once merged. Suggest replacing with the evergreen invariant: "any regression to wrong ordering would produce max |output| ≈ poison_value." Same for the SKILL.md parenthetical.

9. Line-number references in SKILL.md (test_gqa.py:1501, :1761) will drift on the next change to that file. Reference the test function names instead.

10. MlasEltwiseAdd<float>(addend, scores, scores, count) aliased pointers. Verified safe — MLAS loads both operands before storing, and the same pattern exists at attention_helper.h:37 and in dynamic_quantize_matmul.cc:316. Not an issue, just noting it was checked.

✅ Praise

• The fold_mask_into_gemm / softcap_active factoring with the 8-line "why" comment is the right density for a correctness-sensitive invariant.
• Poison-V tests are a much stronger oracle than “high-softcap ≈ identity” checks.
• Pipeline diagram in SKILL.md (stage 0→1→2→3 with the integer mapping right below) is the clearest part of the diff.
• Deep-memcpy snapshots before in-place mutation correctly avoid the alias/lifetime traps that this kind of staging refactor often introduces.
• Leading the spec on a clear correctness bug (rather than waiting for ONNX v1.22 to make the as-shipped opset-23 behavior officially wrong) is the right product call.

Verification done during review

• All four qk_matmul_output_mode × softcap × mask combinations traced by hand — flow is internally consistent.
• Repo grep + 712-model parse confirmed no in-tree serialized models carry qk_matmul_output_mode (so CI won’t catch a regression on the chosen interpretation — finding #2 is the safety net).
• CUDA path: confirmed rename-only plus existing host guard rejecting modes beyond kNone/kQK; no new CUDA snapshot behavior introduced.
• Confirmed onnx/onnx#7867 and #7913 are post-release errata to opset 23 — the ordering/numbering this PR implements matches the corrected spec text, not the as-shipped opset-23 spec.

Reviewers: Opus 4.7 (lead), GPT-5.3-Codex (code), GPT-5.5 (critical), Sonnet 4.6 (readability).

[tianleiwu]

Review Summary

The core fix is correct and well-motivated: the CPU EP now applies softcap before attn_mask/attn_bias, matching both the CUDA EP and the ONNX v23/v24 spec (onnx/onnx#7867 + #7913). The fold_mask_into_gemm refactoring is clean — it preserves FMA-fused numerics for the no-softcap path and only takes the explicit AddInPlace path when ordering matters.

Correctness notes (verified):

• fold_mask_into_gemm guards are correct: disabled when softcap_active or snapshot_needs_pre_mask.
• nonpad_kv_seqlen placement after softcap is correct — the overwrite with mask_filter_value ensures positions are effectively masked regardless of any prior softcap transformation.
• AddInPlace<MLFloat16> scalar fallback has no overflow risk (intermediate computation in fp32, inputs bounded by softcap range).
• Snapshot ordering matches the pipeline stages exactly.

Two test coverage gaps (endorsed from prior review threads) remain the main blocker for full confidence:

1. Mode 1 (kPostSoftCap) is not differentiated from mode 0 (kQK) when softcap is active — the test uses softcap=NaN where they are observationally identical.
2. No test combines nonpad_kv_seqlen with softcap > 0 to guard the reordered path.

These gaps are more important here than usual: because ORT intentionally leads the spec (applying #7913's numbering to both opset 23 and 24), the test suite is the de-facto contract for the chosen semantics.

Overall the fix is the right call. Addressing the two test gaps would let this merge with high confidence.

[tianleiwu]

Inline findings (posted as body due to GitHub API inline-comment issue on this PR):

onnxruntime/core/providers/cpu/llm/attention.cc line ~469 (fold_mask_into_gemm):
Good design: cleanly separates the legacy FMA path (no numeric regression) from the spec-correct path (softcap then explicit mask add). The three conditions are exactly the cases that must avoid folding:

• softcap_active: mask must come after softcap per spec
• snapshot_needs_pre_mask: kQK/kPostSoftCap snapshots must not include the mask

Verified correct.

onnxruntime/core/providers/cpu/llm/attention.cc line ~107 (AddInPlace):
The MLAS fp16 limitation rationale is well-documented. If a future commit adds x86 fp16 MLAS support, this fallback should be gated behind a runtime check rather than being unconditional.

onnxruntime/test/providers/cpu/llm/attention_op_test.cc line ~1552:
Endorsing the earlier feedback: since this test uses softcap = NaN (disabled), mode 1 (kPostSoftCap) produces the same tensor as mode 0. A differentiating test with softcap > 0 would lock in the new semantics.

[titaiwangms]

Thanks @tianleiwu for the explicit blockers and to @copilot-pull-request-reviewer for the inline catches. Round addresses both blockers + every minor:

BLOCKER 1 — mode-1+softcap differentiating coverage. Without softcap > 0, the qk_matmul_output_mode 1↔2 enum swap is observationally equivalent to a rename — no scenario distinguishes the two integer mappings. Two new tests now do: C++ Attention_QkMatmulOutputMode_PostSoftCap_WithSoftcap_CPU and Python test_cpu_attention_qk_matmul_output_mode_post_softcap_with_softcap_fp32. They construct a case where mode 1 under post-#7913 numbering yields softcap * tanh(scale*QK/softcap) and under pre-#7913 numbering would yield [raw_qk, lowest()]. Both force CPU EP; snapshot tolerance 1e-5.

BLOCKER 2 — softcap + nonpad_kv_seqlen leakage. The latent ordering bug (sentinel pre-softcap → tanh squashes it → ~25% leakage to padded positions) is now pinned by C++ Attention_NonPadKVSeqLen_WithSoftcap_NoLeakage_CPU and Python test_cpu_attention_softcap_nonpad_kv_seqlen_no_leakage_prompt_fp32, using poison V (=1000) at padded positions; bounded output (≈ 1.0) confirms the post-fix ordering.

Spec-leading documentation (R-2 #1 / your spec-versioning concern). A 12-line IMPORTANT block now opens the QKMatMulOutputMode enum in attention_parameters.h stating ORT intentionally LEADS the bundled ONNX submodule (v1.21.0, predates onnx/onnx#7913); the v1.22 bump will retroactively make ORT spec-correct with no behavior change.

ONNX backend node tests. 20 fixtures in cmake/external/onnx encode the pre-#7867/#7913 ordering and disagree with our impl. Added them to TestCase.cc::GetBrokenTests with the unblock criterion cmake/external/onnx >= v1.22; pattern matches the existing cast_*_INT4 precedent (bare names, exact strings). Will be removed in the same PR that bumps the submodule.

Bot inlines all applied: typo defintion → definition, nonpad_kv_seqlen masking comment now correctly cites mask_filter_value<T>() (with reference to core/mlas/lib/eltwise.cpp for the MLAS finite-input rationale) instead of "guaranteed -inf", RunTest4D regen comment now notes the opset 23/24 shared-ordering, and the CPU softcap-mask docstring no longer claims fp16 unfused-attention is unsupported.

Verified locally: 60/60 AttentionTest.* PASS (was 58/58 + 2 new). Pre-patch 11 attention node-test failures → post-patch 0 (with the skip-list active). Zero CUDA production code changes — CPU-only fix. lintrunner clean.

Pushed as 2 additive commits, no force-push, so the review history stays intact:

• 87c7e89 — tests + bot minors + spec-leading doc block
• 0cd61f9 — ONNX backend node-test skip-list

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

[titaiwangms]

Review (4-agent fan-out: code-reviewer + critical-reviewer + readability-reviewer + me)

🔴 Critical

None.

🟡 Major (worth addressing before merge)

1. Missing test: kPostMaskBias snapshot + nonpad_kv_seqlen. The new code correctly takes the mode-2 snapshot AFTER nonpad masking is applied, but no test exercises that combination. A future regression that moves the snapshot earlier would slip past the Y-only assertions. Add one CPU test: softcap > 0 + nonpad_kv_seqlen < kv_seq_len + qk_matmul_output_mode=2 + assert padded columns hold the mask sentinel.

2. Spec-leading essay needs an explicit TODO(onnx-v1.22) cleanup hook. The 25-line "IMPORTANT" comment in attention_parameters.h and the 18-entry skip block in TestCase.cc (and the matching 7-entry block in onnx_backend_test_series_filters.jsonc) explain why the divergence exists but give a future maintainer no actionable trigger. When cmake/external/onnx is bumped, all three blocks become silent stale debt. Add at the top of each:

   // TODO(onnx-v1.22): When cmake/external/onnx is bumped to a release that
   // includes onnx/onnx#7867 + #7913, (1) delete this block, (2) remove the
   // matching skip-list entries in TestCase.cc and
   // onnx_backend_test_series_filters.jsonc.
   

   Lowest-cost way to make the deliberate divergence retrievable when it matters.

3. Confusing wording: "FMA-fused numerics that pre-spec-fix tests were calibrated against" (attention.cc:452-453). Implies FMA is mandatory and that tolerances would break on a non-FMA build. Suggested:

   "Keeps the numerical path identical to the pre-fix implementation on the no-softcap case, so pre-existing test expected values remain valid."

4. Optional output_qk subtly perturbs primary Y numerics. fold_mask_into_gemm is disabled when kQK or kPostSoftCap is requested, so the same (softcap=0, mask present) input runs through beta=0 + explicit-add instead of beta=1 FMA. Mathematically equivalent but not bitwise — adds an "observer effect" to a diagnostic output. Either:

   • acknowledge with a short comment near snapshot_needs_pre_mask, or
   • add a regression test confirming Y matches with/without output_qk on the masked, softcap-off case.

🔵 Minor

1. assert output_qk in (0,1,2,3) (common.py:170) is removed under python -O. Replace with if output_qk not in (0,1,2,3): raise ValueError(...).
2. snapshot_needs_pre_mask (attention.cc:466) conflates two distinct semantics — kQK blocks the fold because it's pre-softcap (hence also pre-mask), while kPostSoftCap blocks because it's only pre-mask. Rename to snapshot_is_before_mask_stage or add an inline note clarifying both branches.
3. SKILL.md "(post-fix; pre-fix)" reads like postfix/prefix operator notation. Reword to "before / after PR Fix CPU Attention softcap/attn_mask ordering (onnx#7867, #7913) + consolidate CUDA spec coverage tests from #28371 #28379".
4. Stale line-number refs in test_gqa.py:1934-1937 ("at line 1501, … at line 1761") will go stale on any insert. Use Class.method names instead.
5. Cross-EP softcap validation drift: CUDA enforces softcap >= 0, CPU silently treats negative as disabled (softcap_active = parameters.softcap > 0.0f). PR touches both files; trivially align by moving the check into the shared Attention<T> constructor or the helper.
6. SKILL.md inline mode mapping (0 = …, 1 = …) should be a bullet list to match the rest of section 4's style — faster to skim.

↪️ Follow-up (out of scope, surfacing for tracking only)

• ORT attention_helper.h rejects shorter attn_mask KV dim with nonpad_kv_seqlen, but ONNX v24 spec allows it. Real perf hit for users with large external KV caches.
• nonpad_kv_seqlen == 0 produces inconsistent output across EPs (CPU = mean(V), CUDA MEA = zeros). Pre-existing TODO; pick one and enforce.
• CUDA validates nonpad_kv_seqlen values via device asserts rather than host-side fail-closed validation — resilience concern under adversarial input.

✅ What's right

• Fix logic is correct. fold_mask_into_gemm preserves the FMA-fused path on the no-softcap, no-snapshot case (where pre-existing tests live) and switches to explicit-add exactly when spec correctness demands it.
• Enum rename is clean across all consumers — no stale kQKMask/kQKSoftCap/kQKSoftMax references in ORT production code.
• CUDA attention.cc diff is genuinely cosmetic-only — no behavior change, as claimed.
• AddInPlace<MLFloat16> doc comment (attention.cc:97-105) is exemplary: names the failing function, the build condition, file:line for context, and the runtime consequence.
• ASCII pipeline diagram in SKILL.md §4 is the clearest single artifact in the PR.
• Skip-list rationale comments correctly use "expected" and cite both upstream PRs, preventing future triage as bugs.
• Spec-leading the kernel ahead of the bundled ONNX submodule is an intentional design choice (confirmed) — the v1.22 bump becomes a no-op.

Recommendation

Approve once Major #1 and Major #2 are addressed. Major #3 and #4 are wording/test-hygiene improvements. Minors are take-or-leave.

[titaiwangms]

Thanks @copilot-pull-request-reviewer for the second-round review and to the internal fan-out for surfacing the future-stale-debt risk on the spec-leading blocks. All addressed across 3 additive commits (no force-push) — branch tip is now 0b4c75a267.

Bot review 4238940490:

• B1 — cpu/llm/attention.cc:226 ENFORCE message. Updated to enumerate -1 (absent), 0, 1, 2, or 3 so the optional-output case isn't misleading when debugging.
• B2 — attention_op_test.cc:1584 internal-path leak. The reference to lead-39245992/pr1v2-oracle-verify.py (a session-local artifact that doesn't exist in the repo) is scrubbed. The independent-verification claim and the ~6e-7 tolerance numbers are preserved as a repo-local note.

Internal review Major items:

• Major Set up CI with Azure Pipelines #1 — missing kPostMaskBias × softcap × nonpad_kv_seqlen coverage. New CPU test in both C++ (Attention_QkMatmulOutputMode_PostMaskBias_WithSoftcap_NonPadKv_CPU) and Python (test_cpu_attention_qk_matmul_output_mode_post_mask_bias_softcap_nonpad_kv_fp32). Snapshot-discriminating: Δ≈1e9 at position 1 between pre-fix (snapshot before nonpad mask) and post-fix (snapshot after). Local: 61/61 AttentionTest.* PASS, 5/5 Python.
• Major Remove vsts test runner in cmake file #2 — TODO(onnx-v1.22) cleanup hooks. Added at all 3 spec-leading sites: attention_parameters.h (enum doc block), onnxruntime/test/onnx/TestCase.cc (C++ skip-list), and onnxruntime/test/testdata/onnx_backend_test_series_filters.jsonc (Python skip-list). Each hook explicitly lists the other two siblings so a future submodule-bumper can find them all.
• Plus minor cosmetic phrase scrubs and a stale onnx/onnx#7865 → #7867 reference fix in common.py:706.

Spec compliance independently re-audited at this tip:

• CPU pipeline: scale·QK → softcap → + attn_mask/bias → nonpad_kv_seqlen mask → softmax → @ V (matches post-Clean up CPU kernel definition for opset 13 Pad #7867/Topo sort the model before saving #7913 ordering).
• CUDA attention.cc diff is byte-identical in logic — cosmetic-only, as previously claimed.
• qk_matmul_output_mode enum mapping matches post-Topo sort the model before saving #7913 numbering (kPostSoftCap = 1, kPostMaskBias = 2).

CI @ 0b4c75a267: sole failure is Linux_TRT_Minimal_CUDA_Test_CI (Azure infra noise — same ~1-minute fast-fail pattern hits every open PR). All real test pipelines green.

[titaiwangms]

Re-reviewed update tip 0b4c75a267 (3 additive commits since bfe33d3d08). Both Major findings I raised earlier are addressed:

✅ Major #1 — kPostMaskBias × softcap × nonpad_kv_seqlen coverage. New C++ Attention_QkMatmulOutputMode_PostMaskBias_WithSoftcapAndNonpad_CPU (attention_op_test.cc +90) and Python mirror in test_gqa.py (+91) close the matrix cell. The snapshot pin is well-constructed:

• Q=[1,0,0,0], K[i]=[a_i,0,0,0] with head_size=4 → scale=0.5 → raw QK = [0.5, 1.0, 0.5, 0.5]
• Validates all four positions independently: pos 0 valid → tanh(0.5); pos 1 valid+masked → tanh(1.0)+(-1e9); pos 2,3 padded → lowest() sentinel
• Final-Y assertion Y == V[0] == [1,1,1,1] confirms no leakage from poison V[2,3]=1000
• The use of large_neg = -1e9 instead of IEEE -inf is a deliberate (and well-commented) choice to keep the snapshot tolerance tractable while preserving the softmax-equivalence property.

One small note: the snapshot at padded positions (pos 2, 3) verifies the nonpad sentinel was applied, but does not independently distinguish softcap-then-mask vs. mask-then-softcap ordering at those positions (since nonpad overwrites unconditionally). The valid positions (0, 1) carry that ordering check, so this isn't a coverage gap — just calling out that the test's discriminating power is concentrated in pos 0/1.

✅ Major #2 — TODO(onnx-v1.22) cleanup hooks. Added at all three spec-leading sites with cross-references so a future submodule-bumper can find all siblings: attention_parameters.h (enum doc), TestCase.cc (C++ skip-list), onnx_backend_test_series_filters.jsonc (Python skip-list). Good pattern.

✅ Minors addressed: kNone-aware error message in attention.cc:226, internal-path scrub in Attention4DWithPastAndPresentQkMatmul comment, stale onnx#7865 → #7867 in common.py. Reference vectors in the scrubbed test are byte-identical (only the comment changed).

⏸ Remaining Minor follow-ups (none blocking):

• "FMA-fused numerics ... calibrated against" wording in attention.cc:452-453 could be tightened
• output_qk observer-effect risk on primary Y numerics (mode-3 Y/probs aliasing) — could use a regression assertion or a defensive comment
• assert output_qk in (0,1,2,3) in common.py:170 — raise ValueError is friendlier for test-debugging
• snapshot_needs_pre_mask naming conflates "snapshot wants the pre-mask state" vs "snapshot must be taken before the AddInPlace mutation"
• Stale line-number refs in test_gqa.py docstrings (1501, 1761) likely shifted with the +91 insertion
• Cross-EP softcap validation drift: CUDA rejects softcap < 0 while CPU silently accepts

Spec-correctness is independently re-verified (CPU pipeline matches post-#7867/#7913 ordering, CUDA diff is cosmetic, enum mapping kPostSoftCap=1, kPostMaskBias=2 matches post-#7913). LGTM with the remaining minors as optional follow-ups — nothing blocking merge from my side.

[tianleiwu]

Review Summary — APPROVE

The core CPU attention fix is correct: the reordered pipeline now applies scale*QK → softcap → +mask → softmax, matching both the CUDA EP and the ONNX v23/v24 spec (onnx/onnx#7867 + #7913). The fold_mask_into_gemm refactoring cleanly preserves FMA-fused numerics for the no-softcap path while enabling explicit AddInPlace only when ordering matters.

Both gaps raised in my prior review are now addressed:

1. Mode 1 (kPostSoftCap) is differentiated from mode 0 (kQK) with softcap > 0 — via Attention_QkMatmulOutputMode_PostSoftCap_WithSoftcap_CPU (C++) and test_cpu_attention_qk_matmul_output_mode_post_softcap_with_softcap_fp32 (Python).
2. nonpad_kv_seqlen + softcap > 0 is covered — via Attention_NonPadKVSeqLen_WithSoftcap_NoLeakage_CPU (C++) and test_cpu_attention_softcap_nonpad_kv_seqlen_no_leakage_prompt_fp32 (Python).

Strengths:

• Enum rename to kPostSoftCap/kPostMaskBias/kPostSoftMax makes stage ordering self-documenting.
• The io_binding.synchronize_outputs() addition fixes a real cross-stream race in the Python test harness.
• Skip lists are well-cited with clear TODO for onnx-v1.22 unblock.
• SKILL.md pipeline diagram is immediately readable.

Non-blocking observation: The static_cast<int>(probs_matrix_size) in ComputeAttentionSoftcapInplace calls is inherited from pre-existing code and could theoretically overflow for very large q_seq * total_seq > INT_MAX. Not a regression here — worth a follow-up SafeInt conversion if CPU attention ever targets sequences > ~46k tokens.

Resolving both previously-open threads; test coverage gaps are closed.