Skip to content

Fix A5 GEMV verifier for aligned ACC rows#651

Open
HecreReed wants to merge 2 commits into
hw-native-sys:mainfrom
HecreReed:codex/a5-gemv-verifier-fix
Open

Fix A5 GEMV verifier for aligned ACC rows#651
HecreReed wants to merge 2 commits into
hw-native-sys:mainfrom
HecreReed:codex/a5-gemv-verifier-fix

Conversation

@HecreReed
Copy link
Copy Markdown
Collaborator

Summary

  • stop reusing the A5 matmul static M/N shape check for GEMV-family ops
  • keep the A5 left/right/acc layout+slayout enforcement shared between matmul and GEMV
  • add an A5 lit regression covering pto.tgemv, pto.tgemv.acc, and pto.tgemv.bias with aligned ACC rows but valid_shape[0] == 1

Validation

  • git diff --check
  • local macOS configure succeeded with cmake -G Ninja -S . -B build-local ...
  • full local ptoas build is currently blocked by pre-existing macOS -Wcovered-switch-default errors in lib/PTO/IR/PTOSyncUtils.cpp and lib/PTO/IR/PTOTypeDefs.cpp

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors tile layout verification for the a5 architecture by introducing the verifyMadTileLayoutsA5 function and updating verifyMatTileOperandsA5 and verifyGemvTileOperandsA5 to use it. It also adds a new LIT test for tgemv operations. Feedback suggests improving the robustness of the new verification function by checking each operand's type independently rather than skipping all layout checks if a single operand is not a TileBufType.

Comment thread lib/PTO/IR/PTO.cpp
Comment on lines +3139 to +3140
static LogicalResult verifyMadTileLayoutsA5(Operation *op, Type lhsTy,
Type rhsTy, Type dstTy) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The current implementation of verifyMadTileLayoutsA5 (refactored from verifyMatTileOperandsA5) uses an "all-or-nothing" check for TileBufType operands. If any of the operands is not a TileBufType (e.g., a MemRefType during intermediate lowering stages), all layout checks are skipped for the remaining operands. It would be more robust to check each operand independently if it is a TileBufType.

@reedhecre
Copy link
Copy Markdown

reedhecre commented May 11, 2026

Codex Review

该评论由 review 机器人自动更新。

  • PR: Fix A5 GEMV verifier for aligned ACC rows #651 Fix A5 GEMV verifier for aligned ACC rows
  • Author: HecreReed
  • Base/Head: main / codex/a5-gemv-verifier-fix
  • Head SHA: 2b186ac481ee
  • Trigger: PR 有新提交
  • Generated At: 2026-05-11T02:23:45Z
  • Previous Head SHA: 2ffb991312fe
  • Status: completed

Summary

A5 GEMV verifier regressed in this PR: the new helper call fixes aligned ACC rows, but it also drops prior range and logical-shape validation for all A5 GEMV variants.

Findings

  1. P2 A5 GEMV no longer rejects out-of-range valid sizes lib/PTO/IR/PTO.cpp:3214

verifyGemvTileOperandsA5() used to delegate to verifyMatTileOperandsA5(), which in turn enforced the m/k/n valid-size bounds from verifyMatTileOperandsA2A3() ([1, 4095]). After this change it returns verifyMadTileLayoutsA5(...) directly, so every A5 GEMV variant now loses that verifier coverage. That means pto.tgemv, pto.tgemv.acc, pto.tgemv.bias, and the tgemv.mx* ops can now accept k/n values above 4095 that were previously rejected and are still documented as invalid, which is a real verifier regression with runtime-risky inputs.

  1. P2 A5 GEMV shape compatibility checks were over-relaxed lib/PTO/IR/PTO.cpp:3214

This refactor fixes aligned ACC rows by bypassing verifyMatTileOperandsA5(), but that call was also the only A5 GEMV path that checked full logical lhs[M,K] / rhs[K,N] / dst[M,N] compatibility through getMatmulLogicalShapeVec(). The replacement only checks a subset of valid_shape relationships, so malformed A5 GEMV descriptors can now slip through whenever one side uses dynamic or padded extents. For example, a dst tile with a mismatched static column shape but dynamic v_col now passes verification even though the GEMV contract still requires rhs.cols == dst.cols. That widens the accepted IR beyond the intended "aligned ACC rows" relaxation and can reach codegen/runtime with inconsistent tile metadata.

@HecreReed HecreReed marked this pull request as ready for review May 11, 2026 02:48
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2b186ac481

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread lib/PTO/IR/PTO.cpp
if (failed(verifyGemvTileOperandsA2A3(op, lhsTy, rhsTy, dstTy)))
return failure();
return verifyMatTileOperandsA5(op, lhsTy, rhsTy, dstTy);
return verifyMadTileLayoutsA5(op, lhsTy, rhsTy, dstTy);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reinstate GEMV K/N bound checks on A5

Changing A5 GEMV verification to call only verifyMadTileLayoutsA5 drops the [1, 4095] valid-size guard that previously came from verifyMatTileOperandsA2A3 via verifyMatTileOperandsA5. As a result, pto.tgemv* now accepts static valid_shape values such as K=5000 or N=5000 on A5, even though matmul-family verification still treats those sizes as out of range; this can allow invalid TGEMV dimensions through verification and fail later in lowering/codegen.

Useful? React with 👍 / 👎.

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