Skip to content

Fix sliding-window sequence length mask detection#2436

Merged
umangyadav merged 2 commits into
developfrom
users/umayadav/fix-sliding-window-mask-detection
Jul 24, 2026
Merged

Fix sliding-window sequence length mask detection#2436
umangyadav merged 2 commits into
developfrom
users/umayadav/fix-sliding-window-mask-detection

Conversation

@umangyadav

@umangyadav umangyadav commented Jul 23, 2026

Copy link
Copy Markdown
Member

Summary

  • validate that sliding-window masks reference an i32 current-sequence-length block argument
  • preserve and reconcile sequence-length clip bounds when folding sliding-window and KV-cache masks
  • add regression coverage for invalid operands and mismatched sequence lengths or clip bounds

Test plan

  • ninja rocmlir-opt
  • TosaToRock attention lit tests (12/12 passed)
  • git clang-format --diff origin/develop
  • ninja check-rocmlir (11,802 passed; one unrelated tuningRunner-gemm.py infrastructure failure because the build directory was not found)
sliding-window-mask-before-after sliding-window-mask-reconciliation

Validate and reconcile sequence lengths so unsupported masks are not folded and clipping semantics are preserved.

Co-authored-by: Cursor <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens rocMLIR’s TOSA→Rock attention mask folding by ensuring sliding-window mask detection is anchored to a validated i32 current-sequence-length block argument, and by reconciling (or rejecting) mismatched sequence-length clamp bounds when combining sliding-window and KV-cache masking.

Changes:

  • Enhance sliding-window mask detection to require the non-constant operand resolves to an i32 block argument (currentSeqLen), and to extract any surrounding clip bounds.
  • Reconcile sliding-window and KV-cache currentSeqLen + clip bounds after all select masks are analyzed, making results independent of select nesting order.
  • Add a regression test covering a non-sliding-window “wrong operand” case and a no-KV-cache sliding-window case that must preserve the validated clip.

Reviewed changes

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

File Description
mlir/lib/Conversion/TosaToRock/TosaToRock.cpp Adds validated sliding-window seq-len resolution + clip tracking, and reconciles it with KV-cache-derived seq-len/clip constraints.
mlir/test/Conversion/TosaToRock/tosa-to-rock-attention-sliding-window-neg.mlir Adds regression coverage for correct sliding-window adoption of clipped currentSeqLen, and for rejecting masks with an unrelated operand.

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

@umangyadav umangyadav added the claude-review Trigger automated PR review by claude[bot]; auto-removed after the run. label Jul 23, 2026

@rocmlir-pr-reviewer rocmlir-pr-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE -- submitted as COMMENT (automated reviews are advisory)  ·  Findings: 0 (0 Critical, 0 Major, 0 Minor)


Scope

Fixes sliding-window sequence-length mask detection in the TosaToRock attention rewrite (mlir/lib/Conversion/TosaToRock/TosaToRock.cpp) and adds Lit coverage (mlir/test/Conversion/TosaToRock/tosa-to-rock-attention-sliding-window-neg.mlir). Previously trySlidingWindowPattern returned only a window size without checking that the non-constant add operand is really an i32 currentSeqLen block argument, so an unrelated greater(x - const, col) could be misclassified. The PR validates that operand, threads through detected clip bounds, and reconciles the KV-cache vs. sliding-window seq-len block arg and clips after all masks are analyzed.

Findings

No blocking issues found.

Notes

  • Correctness spot-checks pass: the aggregate initializer at TosaToRock.cpp:2622 matches the updated SeqLenMaskResult field order at TosaToRock.cpp:2110; SlidingWindowResult/slidingWindowSeqLen are consumed only at the analyze site (2588) and the reconciliation block (2660-2675). The new sameSeqLenBlockArg and clip storage mirror the existing KV-cache design (both store the resolved block arg plus optional clip bounds), and the reconciliation re-resolves both operands through seqLenSkip before comparing, so it cannot dereference a null value.
  • Conservative-failure behavior is intentional and safe: when the KV-cache and sliding-window masks reference different block args or different clip bounds, getSeqLenMask returns failure() and abandons the whole rock.attention rewrite rather than emitting a possibly-incorrect op.
  • Minor (non-blocking, not a checklist item): the file is named ...-neg.mlir, but the first function sliding_window_no_kvcache is a positive case (it does detect a sliding-window mask and adopts its seq-len); only the second function is a true negative. Naming is slightly misleading but harmless.

CI status

No non-self CI failures. C/C++ premerge checks, Python format and lint checks, and Python performance script tests all pass; the in-progress review check is this auto-review pipeline and is expected.

@rocmlir-pr-reviewer rocmlir-pr-reviewer Bot removed the claude-review Trigger automated PR review by claude[bot]; auto-removed after the run. label Jul 23, 2026
Cover divergent current-sequence-length operands and clip bounds so unsupported mask combinations remain explicit.

Co-authored-by: Cursor <[email protected]>
@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.48936% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
mlir/lib/Conversion/TosaToRock/TosaToRock.cpp 91.49% 0 Missing and 4 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2436      +/-   ##
===========================================
+ Coverage    82.57%   83.57%   +1.00%     
===========================================
  Files          120      120              
  Lines        42852    42977     +125     
  Branches      7110     7150      +40     
===========================================
+ Hits         35381    35914     +533     
+ Misses        4815     4492     -323     
+ Partials      2656     2571      -85     
Flag Coverage Δ
gfx120x 83.42% <91.49%> (+0.90%) ⬆️
gfx950 83.38% <91.49%> (+1.04%) ⬆️
mfma 83.42% <91.49%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
mlir/lib/Conversion/TosaToRock/TosaToRock.cpp 79.39% <91.49%> (+0.98%) ⬆️

... and 32 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bogdan-petkovic bogdan-petkovic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@umangyadav
umangyadav merged commit d97f81c into develop Jul 24, 2026
12 of 19 checks passed
@umangyadav
umangyadav deleted the users/umayadav/fix-sliding-window-mask-detection branch July 24, 2026 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants