fix: avoid fusing identity transpose into Gemm - #32435
Xavier Dupré (xadupre) merged 4 commits into
Conversation
Signed-off-by: luozijian <luozijian0924@gamil.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
The newly guarded Gemm-output transpose path lacks regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Prevents GemmTransposeFusion from treating identity transposes as matrix transposes.
Changes:
- Validates explicit and default transpose permutations before fusion.
- Adds regression coverage for identity transpose on Gemm input B.
File summaries
| File | Description |
|---|---|
onnxruntime/core/optimizer/gemm_transpose_fusion.cc |
Restricts fusion to genuine 2D matrix transposes. |
onnxruntime/test/optimizer/graph_transform_test.cc |
Tests preservation of an identity weight transpose. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: luozijian <luozijian0924@gamil.com>
|
Addressed the review note in commit 8b7e6ff by adding GemmTransposeFusionDoesNotFuseIdentityTransposeAtOutput. This covers the output-side identity transpose guard; both input- and output-side identity transposes now have regression coverage. lintrunner -a and git diff --check pass locally. The native ONNX Runtime test binary is not available in this checkout, so the gtest itself remains for upstream CI. |
|
It is probably worth checking it is not happening somewhere else as well. |
|
Thanks for the suggestion. I audited the other transpose-related optimizer paths in this checkout. |
|
Thanks for the approval and follow-up. The full CI exposed a compile issue in the new regression tests: the shape-only MakeInput calls used a single-brace initializer and did not match ModelTestBuilder's overload. Fixed in commit 031cc67 by using the existing double-brace shape form; the fix is now pushed to the PR branch. The affected build failures all pointed to those four calls. CLA remains pending. |
|
@microsoft-github-policy-service agree |
Description
Prevent
GemmTransposeFusionfrom folding aTransposeintoGemmunless it is a real two-dimensional matrix transpose. Identity transposes are now left unchanged, and defaultpermhandling remains supported for known two-dimensional inputs.Add a graph-transform regression test covering an identity transpose on the Gemm weight input.
Motivation and Context
Fixes #32418.
An identity
Transpose(perm=[0, 1])feeding a Gemm was incorrectly treated as a matrix transpose. The optimizer removed the node and settransB=1, changing the graph semantics and causing an invalid bias-shape error for non-square weights. The fix keeps the identity transpose in the graph and avoids changing Gemm attributes.Validation
git diff --checkpassed.Gemm: Invalid bias shape for broadcastwith basic optimization; the optimized graph containedtransB=1.AI assistance disclosure
This patch was developed with AI assistance. The issue, optimizer logic, regression test, and validation evidence were reviewed against the repository source and contribution guidance.