Skip to content

[NFC] Apply clang-format to GridwiseAttnToBlockwise.cpp - #537

Merged
umangyadav merged 1 commit into
developfrom
users/umayadav/attn-format-nfc
Sep 24, 2026
Merged

umangyadav merged 1 commit into
developfrom
users/umayadav/attn-format-nfc

Conversation

@umangyadav

@umangyadav umangyadav commented Sep 23, 2026 •

Copy link
Copy Markdown
Member

Stack (merge bottom-up):

  1. [NFC] Apply clang-format to GridwiseAttnToBlockwise.cpp #537 — [NFC] clang-format of GridwiseAttnToBlockwise.cpp (base develop)
  2. Handle fully masked attention rows in kernel lowering #534 — kernel empty-row guards (base [NFC] Apply clang-format to GridwiseAttnToBlockwise.cpp #537)
  3. Recombine fully masked split-KV attention partials on the host #535 — host split-KV recombination (base Handle fully masked attention rows in kernel lowering #534)

Summary

Pure clang-format run over mlir/lib/Dialect/Rock/Transforms/GridwiseAttnToBlockwise.cpp. No functional change.

Split out of #534 so that the fully-masked-attention-row work reviews as functional-only.

What changed

Whitespace and comment rewrapping only:

  • Wrapped over-long parameter lists (expSubstractMaxFromGemm0, updateRowSum, scaleFinalOutput, createAttentionRowStateCorrections).
  • Broke long assignment expressions across lines.
  • Stripped trailing whitespace.
  • Rewrapped comments to the 80-column limit.

Verification

  • Stripping all whitespace from the before/after files yields byte-identical token streams (the only residual delta is the // markers added by comment rewrapping).
  • Rock dialect + rocmlir-gen lit suites: 176/176 pass.

Pure clang-format run over the existing file. No functional change; the
diff is whitespace and comment rewrapping only. Split out of the fully
masked attention row work so that change reviews as functional-only.

Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot AI 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.

Copilot review overview

🟢 Approval recommended

Formatting-only changes with no unresolved review comments.

Review effort: Lite
Findings: None

What changed in this PR

Applies clang-format to GridwiseAttnToBlockwise.cpp with no functional changes.

Changes:

  • Reformats signatures, expressions, assignments, and comments.
  • Removes trailing whitespace and normalizes spacing.
File Description
mlir/​lib/​Dialect/​Rock/​Transforms/​GridwiseAttnToBlockwise.cpp Formatting-only updates to attention lowering code.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread mlir/lib/Dialect/Rock/Transforms/GridwiseAttnToBlockwise.cpp

@rocmlir-pr-reviewer rocmlir-pr-reviewer Bot 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.

Verdict: COMMENT  ·  Findings: 1 (0 Critical, 1 Major, 0 Minor)


Scope

Single-file [NFC] clang-format run over mlir/lib/Dialect/Rock/Transforms/GridwiseAttnToBlockwise.cpp (PR #537, bottom of a three-PR stack with #534 / #535). No other files touched.

Findings

I walked every hunk in diff.patch and confirmed the change is genuinely non-functional: each edit is line wrapping, trailing-whitespace removal, comment reflow to 80 columns, or re-indentation. Spot-checked the places where a formatting pass could plausibly hide a semantic change and found none:

  • expSubstractMaxFromGemm0 / updateRowSum / scaleFinalOutput / createAttentionRowStateCorrections — parameter lists rewrapped, argument order and types unchanged at both declaration and call sites (GridwiseAttnToBlockwise.cpp:199, :227, :281, :312).
  • std::tie(maxRowDiffExp, sumRow, maxRow) = updateRowSum(...) at :1338 — same six arguments, same binding order.
  • The OutOfScopeType::Causal / PrefixCausal case bodies at :465 were under-indented relative to their case labels before; the new indentation is a real readability fix and does not alter the arith::CmpIOp predicates or the assert(prefixOffset != nullptr) guard.
  • The if (...getElementType() != elemTypeOut) body at :1607 was over-indented; only leading whitespace changed.

One process finding: the PR touches a path shared with ROCm/rocMLIR and carries no back-port note. See the inline comment.

Notes

  • Reformatting a shared file is exactly the kind of change that makes future back-ports conflict on every hunk, so the back-port question is more load-bearing here than the [NFC] tag suggests — either land the same format run upstream, or record that the file has diverged past the point of sharing.
  • The (e.g. f16 -> f32) comments at :295 and :325 now break mid-arrow across two lines. That is what clang-format produces for the existing wording and is not a checklist violation; if it bothers you, shortening the prose (rather than adding // clang-format off) keeps the file idempotent under the formatter.
  • No lit/E2E coverage is expected for a formatting-only change, and none is required by the checklist.

CI status

No failing or cancelled checks. detect, py-checks, and copilot-pull-request-reviewer pass; notify-fork-pr is skipped; review is this auto-review job itself, still in progress.

@rocmlir-pr-reviewer rocmlir-pr-reviewer Bot removed the claude-review Request a Claude PR review label Sep 23, 2026
@umangyadav
umangyadav enabled auto-merge (squash) September 23, 2026 23:07
@umangyadav
umangyadav merged commit a1ed440 into develop Sep 24, 2026
13 of 14 checks passed
@umangyadav
umangyadav deleted the users/umayadav/attn-format-nfc branch September 24, 2026 01:04
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.

3 participants