Skip to content

[Rewriter]: Add ∘ MatMul -> Gemm #2356

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 14, 2025

Conversation

AyoubMDL
Copy link
Contributor

@AyoubMDL AyoubMDL commented Jun 1, 2025

A Rewriter rule that transforms MatMul(Add) to Gemm.

@AyoubMDL
Copy link
Contributor Author

AyoubMDL commented Jun 1, 2025

@justinchuby I wrote the rewriter using functions (as in gemm_to_matmul_add). Tell me if you want me to do it using Classes.

@justinchuby justinchuby requested a review from Copilot June 2, 2025 04:58
Copy link
Contributor

@Copilot Copilot AI left a comment

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 introduces a rewriter rule that transforms a MatMul followed by an Add operation into a Gemm operator for streamlined execution. The changes include implementation of the transformation rule in the rewriter module and accompanying tests to validate both standard and edge-case behaviors.

  • Added a new transformation rule in onnxscript/rewriter/matmul_add_to_gemm.py.
  • Introduced tests in onnxscript/rewriter/matmul_add_to_gemm_test.py for regular inputs, initializers inclusion, and incompatible shapes.

Reviewed Changes

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

File Description
onnxscript/rewriter/matmul_add_to_gemm_test.py Test cases ensuring the fusion of MatMul and Add into Gemm are correct.
onnxscript/rewriter/matmul_add_to_gemm.py Implementation of the rewriter rule for converting MatMul+Add to Gemm.
Comments suppressed due to low confidence (1)

onnxscript/rewriter/matmul_add_to_gemm_test.py:12

  • [nitpick] The test class name 'MatMulAddToMatMulTest' is misleading given that the transformation targets Gemm. Consider renaming it to 'MatMulAddToGemmTest' for clarity.
class MatMulAddToMatMulTest(unittest.TestCase):

Copy link

codecov bot commented Jun 2, 2025

Codecov Report

Attention: Patch coverage is 98.80240% with 2 lines in your changes missing coverage. Please review.

Project coverage is 70.36%. Comparing base (321cb41) to head (9b5bec4).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
onnxscript/rewriter/matmul_add_to_gemm_test.py 98.36% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2356      +/-   ##
==========================================
+ Coverage   70.15%   70.36%   +0.20%     
==========================================
  Files         197      199       +2     
  Lines       24985    25152     +167     
  Branches     2669     2681      +12     
==========================================
+ Hits        17529    17698     +169     
+ Misses       6529     6528       -1     
+ Partials      927      926       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@justinchuby
Copy link
Collaborator

@justinchuby I wrote the rewriter using functions (as in gemm_to_matmul_add). Tell me if you want me to do it using Classes.

Would be nice to use the class based approach just for consistency, thanks!

@AyoubMDL AyoubMDL changed the title [Rewriter]: MatMul ∘ Add -> Gemm [Rewriter]: Add ∘ MatMul -> Gemm Jun 3, 2025
@AyoubMDL AyoubMDL marked this pull request as draft June 3, 2025 17:55
@AyoubMDL AyoubMDL force-pushed the matmul_add_to_gemm branch from 90ed11e to 1a8a641 Compare June 12, 2025 21:08
@AyoubMDL
Copy link
Contributor Author

Added the support for transposed inputs

I think it’s possible to detect the number of users of the weight. Sounds like if Transpose isn’t the only consumer of the weight value then we can skip the rule.

I think I dont need to check the users, the rewriter does check the number of users by default (if i am not mistaken)

You can allow the rule to permute input orders

With the current implementation, I don't think I need it, right ?

@AyoubMDL AyoubMDL marked this pull request as ready for review June 12, 2025 21:14
@AyoubMDL AyoubMDL requested a review from justinchuby June 12, 2025 21:14
Copy link
Collaborator

@justinchuby justinchuby left a comment

Choose a reason for hiding this comment

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

Nicely written and tested, thanks!

@AyoubMDL AyoubMDL force-pushed the matmul_add_to_gemm branch from 1a8a641 to b5bd684 Compare June 13, 2025 19:22
@justinchuby justinchuby enabled auto-merge (squash) June 13, 2025 19:51
@justinchuby justinchuby added the merge at lgtm Reviewers can merge when they approve label Jun 13, 2025
@justinchuby justinchuby added this to the 0.3.1 milestone Jun 13, 2025
@justinchuby
Copy link
Collaborator

@titaiwangms could you approve? Thanks

@justinchuby justinchuby merged commit d7974ba into microsoft:main Jun 14, 2025
27 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge at lgtm Reviewers can merge when they approve module: rewriter
Projects
Development

Successfully merging this pull request may close these issues.

4 participants