Fix attention mask traversal for three nested selects - #2437
Conversation
Validate and reconcile sequence lengths so unsupported masks are not folded and clipping semantics are preserved. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Verdict: APPROVE -- submitted as COMMENT (automated reviews are advisory) · Findings: 0 (0 Critical, 0 Major, 0 Minor)
Scope
Fixes TOSA-to-Rock attention mask folding so that three separately-nested select(mask, -inf, scores) ops (prefix-causal, KV-cache, sliding-window) are all recognized and represented as rock.attention attributes, instead of leaving the third mask in the QK elementwise region (which blocked codegen). Touches one C++ file plus two new tests.
Findings
No blocking issues found.
Notes
TosaToRock.cpp:2629-2649: the new peeling loop is well-behaved.foundCountis bounded to [0,3]; every non-breakiteration strictly increases it and the loop condition requires< 3, so termination is guaranteed. ThefoundCount == beforecheck correctly leaves an unrecognized or duplicate mask in the elementwise region by not advancinginputToContinue. The> 0gate preserves the prior "at least one mask required" precondition, so this is a clean generalization of the old single-step logic rather than a behavior change for the 1–2 mask cases.- The seqLen reconciliation block (
:2651-2669) is correctly kept after the loop, so the result stays independent of select nesting order. - Test coverage is appropriate:
tosa-to-rock-attention-three-mask.mlirasserts all four attributes plusCHECK-NOT: tosa.selectinside the elementwise region, and the nightly E2E adds output + LSE verification. Both satisfy the Major "new optimization needs a FileCheck test" and fusion-coverage bullets. - Minor (non-blocking, not raised inline):
foundCount(currentResult)is evaluated a few times per iteration; hoisting into a local would be marginally tidier but is not a checklist item.
CI status
No non-self CI failures. The review pipeline's own checks and the Copilot reviewer are still in progress, which is expected and not a CI failure.
There was a problem hiding this comment.
Pull request overview
This PR improves the TOSA-to-Rock attention matcher so it can fully fold attention graphs that apply three nested select(mask, -inf, scores) masks (prefix-causal, KV-cache, sliding-window), enabling downstream codegen and avoiding lowering failures.
Changes:
- Update
TosaToRockattention matching to iteratively peel and analyze up to three nestedselect(..., -inf, ...)masks. - Add a TOSA-to-Rock regression test ensuring all three masks are folded into a single
rock.attention(with no remainingtosa.selectin the elementwise region). - Add a nightly fusion E2E test covering the three-mask combination and verifying runtime correctness (output + LSE).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| mlir/lib/Conversion/TosaToRock/TosaToRock.cpp | Iteratively traverses chained select masks to detect KV-cache, prefix-causal, and sliding-window masks across three nest levels. |
| mlir/test/Conversion/TosaToRock/tosa-to-rock-attention-three-mask.mlir | New lit regression test asserting all three masks fold into rock.attention attributes and no tosa.select remains in the QK elementwise region. |
| mlir/test/fusion/nightly-misc-e2e/mixr-attention/f16/mixr-attention-sliding-window-kvcache-prefix-causal.mlir | New nightly E2E test exercising the three-mask case and checking both folding and execution results. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ab075d4 to
88f5cdc
Compare
Iteratively peel prefix-causal, KV-cache, and sliding-window masks so the third select does not remain in the elementwise region and trigger "Cannot trace first gemm index for linalg.generic op". Add conversion and nightly E2E regression coverage. Co-authored-by: Cursor <cursoragent@cursor.com>
88f5cdc to
ba15f65
Compare
Use FileCheck DAG assertions so optional attention property ordering does not create brittle test failures. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2437 +/- ##
===========================================
+ Coverage 82.57% 83.71% +1.14%
===========================================
Files 120 121 +1
Lines 42852 43181 +329
Branches 7110 7180 +70
===========================================
+ Hits 35381 36145 +764
+ Misses 4815 4482 -333
+ Partials 2656 2554 -102
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| int before = recognizedMaskCount(currentResult); | ||
| analyzeSelectForSeqLenMask(chainedSelect, currentResult, opsToSkip, | ||
| seqLenSkip); | ||
| // Leave an unrecognized or duplicate mask in the elementwise region. |
There was a problem hiding this comment.
Not necessarily urgent for this PR, but in rocmlirTriton (assuming we are going to port this change there) we may run into errors with the duplicate/unrecognized masks being left in the elementwise region. The regularize* passes cannot sink transforms through non-splat constants (I've seen errors with this before when we were first testing the FusionZoo kernels with the rocmlirTriton prototype)
| // Sliding-window masking is defined relative to currentSeqLen. Reconcile | ||
| // the validated operand after all masks have been analyzed so the result is | ||
| // independent of the select nesting order. | ||
| if (currentResult.slidingWindowSize) { | ||
| if (currentResult.seqLen) { | ||
| if (!sameSeqLenBlockArg(currentResult.seqLen, | ||
| currentResult.slidingWindowSeqLen, seqLenSkip)) | ||
| return failure(); | ||
| // A single attention op cannot represent different clamps for the | ||
| // KV-cache and sliding-window masks. | ||
| if (currentResult.seqLenClipMin != currentResult.slidingWindowClipMin || | ||
| currentResult.seqLenClipMax != currentResult.slidingWindowClipMax) | ||
| return failure(); | ||
| } else { | ||
| currentResult.seqLen = currentResult.slidingWindowSeqLen; | ||
| currentResult.seqLenClipMin = currentResult.slidingWindowClipMin; | ||
| currentResult.seqLenClipMax = currentResult.slidingWindowClipMax; |
There was a problem hiding this comment.
I'm confused here. Doesn't this same block of logic already exist below? Why do we need the same thing twice?
There was a problem hiding this comment.
Is this just a merge conflict artifact from updating the branch before?
Eliminate a branch-update artifact that repeated the same validation block. Co-authored-by: Cursor <cursoragent@cursor.com>
Motivation
Attention graphs can combine prefix-causal, KV-cache, and sliding-window masks as three nested
select(mask, -inf, scores)operations. The matcher previously inspected only the outer select and one chained select, so the third recognized mask remained in the QK elementwise region instead of being represented byrock.attentionattributes.That incomplete folding prevents the attention graph from reaching code generation and can surface with this error signature:
This PR is stacked on #2436 because it relies on that PR's sliding-window sequence-length detection and clip preservation.
Summary
tosa.selectremains.Test plan
tosa-to-rock-attention-three-mask.mlirmixr-attention-sliding-window-kvcache-prefix-causal.mlirgit clang-format --diff origin/users/umayadav/fix-sliding-window-mask-detectionMade with Cursor