Skip to content

Add sliding-window attention validation checks#2438

Open
umangyadav wants to merge 1 commit into
developfrom
users/umayadav/sliding-window-validation
Open

Add sliding-window attention validation checks#2438
umangyadav wants to merge 1 commit into
developfrom
users/umayadav/sliding-window-validation

Conversation

@umangyadav

Copy link
Copy Markdown
Member

Summary

  • reject invalid sliding-window configurations in rocmlir-gen, including negative, missing sequence length, oversized, and non-i32 values
  • cover sliding-window verifier diagnostics for rock.attention and rock.gridwise_attention_accel, including use on non-attention operations

Test plan

  • ninja rocmlir-gen rocmlir-opt
  • focused lit tests for Rock verifier, generator options, and valid sliding-window generation
  • git clang-format --diff origin/develop

Made with Cursor

Reject invalid window configurations before generating IR and cover the corresponding Rock verifier diagnostics.

Co-authored-by: Cursor <[email protected]>
@umangyadav
umangyadav requested a review from causten as a code owner July 23, 2026 19:30
@umangyadav umangyadav added the claude-review Trigger automated PR review by claude[bot]; auto-removed after the run. label Jul 23, 2026
@umangyadav umangyadav self-assigned this Jul 23, 2026

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 strengthens sliding-window attention validation by rejecting invalid rocmlir-gen CLI configurations early and adding negative lit coverage for Rock dialect verifier diagnostics related to sliding-window attention.

Changes:

  • Add rocmlir-gen argument validation for sliding_window_size (non-negative, fits i32, requires current_seq_len, and must not exceed seq_len_k).
  • Add rocmlir-gen lit tests covering the new sliding-window CLI error paths.
  • Add Rock dialect verifier negative tests for rock.attention and rock.gridwise_attention_accel sliding-window diagnostics (including use when not in attention mode).

Reviewed changes

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

File Description
mlir/tools/rocmlir-gen/rocmlir-gen.cpp Adds early validation checks for sliding-window attention CLI options to prevent invalid IR/materialization.
mlir/test/rocmlir-gen/options.mlir Adds lit coverage ensuring rocmlir-gen emits the expected diagnostics for invalid sliding-window configurations.
mlir/test/Dialect/Rock/ops_error.mlir Adds -verify-diagnostics coverage for Rock op verifier errors related to sliding-window usage and constraints.

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

@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

Adds input validation for sliding-window attention in rocmlir-gen (rejects negative, oversized/non-i32, and missing-current_seq_len configurations) and extends lit coverage: options.mlir for the generator diagnostics and ops_error.mlir for the pre-existing rock.attention / rock.gridwise_attention_accel verifier diagnostics. Touches only 3 files (one .cpp, two .mlir tests).

Findings

No blocking issues found.

Notes

  • mlir/tools/rocmlir-gen/rocmlir-gen.cpp:1343-1362: validation logic is correct. slidingWindowSize is cl::opt<int64_t>, so the > INT32_MAX guard is meaningful (a smaller type would make it dead code). The -sliding_window_size=2147483648 test fits in int64_t and correctly trips the i32 guard. Check ordering (non-negative -> i32-fit -> requires-current_seq_len -> not-exceed-seq_len_k) is consistent with the four options.mlir FileCheck prefixes, and all four error strings match verbatim.
  • Semantics are coherent: init(0) = disabled, negative = rejected, positive = enabled (matches the test comments).
  • #include <limits> (line 173) is correctly placed in the stdlib group and lexicographically sorted (limits < tuple < unordered_map) per the include-order Minor bullet.
  • No new files, so no license-header/CMake concerns; no new deps, so librockcompiler_deps.cmake is untouched appropriately. Both edited .mlir files are already-registered test files, so no CMakeLists.txt registration is required.
  • Spot-check limitation: the ops_error.mlir verifier diagnostic strings are asserted against pre-existing verifier code (not modified by this diff); the in-progress premerge lit run is the authoritative check of exact-string matching.

CI status

No non-self checks report fail or cancel. C/C++ premerge checks is still in progress; the auto-review pipeline's own review check being in-progress is expected and not a CI failure.

@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
@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2438      +/-   ##
===========================================
+ Coverage    82.57%   83.60%   +1.03%     
===========================================
  Files          120      120              
  Lines        42852    42952     +100     
  Branches      7110     7144      +34     
===========================================
+ Hits         35381    35906     +525     
+ Misses        4815     4483     -332     
+ Partials      2656     2563      -93     
Flag Coverage Δ
gfx950 83.39% <100.00%> (+1.04%) ⬆️
mfma 83.36% <100.00%> (?)

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

Files with missing lines Coverage Δ
mlir/tools/rocmlir-gen/rocmlir-gen.cpp 87.08% <100.00%> (+0.53%) ⬆️

... and 20 files with indirect coverage changes

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

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.

2 participants