Skip to content

Conversation

@Stonesjtu
Copy link
Contributor

Close #2468

  • Absorbs Affine into Conv:
    • Mul + Add + Conv ==> Conv
    • Conv + Mul + Add ==> Conv
  • Fuse HardSwish:
    • Add + Clip + Div ==> HardSigmoid
    • HardSigmoid + Mul ==> HardSwish
    • Add + Clip + Mul + Div ==> HardSwish (Since the order of operator matters, I have to create different rewrite pattern for this)

May not be generic enough, but works for us in paddleOCRv4 model.

Another question is hardswish is introduced in opset-v14, will onnxscript handles older opset version or rewrite rules take care of this?

@xadupre
Copy link
Member

xadupre commented Jul 31, 2025

We should have a mechanism to enable or disable a rule if the opset does not support the fused operators. But we can manually select the list of rules to apply for now. We should probably opset information in the rule itself. I'll let @gramalingam comment on that.

@codecov
Copy link

codecov bot commented Jul 31, 2025

Codecov Report

❌ Patch coverage is 86.84211% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.62%. Comparing base (9036fab) to head (7b4154d).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
...nnxscript/rewriter/rules/common/_fuse_hardswish.py 76.92% 6 Missing and 6 partials ⚠️
...xscript/rewriter/rules/common/_fuse_conv_affine.py 82.60% 4 Missing and 4 partials ⚠️
...ript/rewriter/rules/common/_fuse_hardswish_test.py 93.47% 1 Missing and 2 partials ⚠️
...pt/rewriter/rules/common/_fuse_conv_affine_test.py 95.45% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2472      +/-   ##
==========================================
+ Coverage   70.37%   70.62%   +0.24%     
==========================================
  Files         218      222       +4     
  Lines       26428    26620     +192     
  Branches     2646     2660      +14     
==========================================
+ Hits        18600    18801     +201     
+ Misses       6922     6899      -23     
- Partials      906      920      +14     

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

@Stonesjtu
Copy link
Contributor Author

@xadupre CI passed, can you review this PR again?

@justinchuby
Copy link
Collaborator

No need to update from main - otherwise we will have to re-trigger CI. Thanks

@Stonesjtu
Copy link
Contributor Author

@justinchuby any other suggestions?

@justinchuby justinchuby self-assigned this Aug 28, 2025
@justinchuby
Copy link
Collaborator

Sorry for the delay. I was waiting for @gramalingam to take a look.

@Stonesjtu We recently refactored the rules and found a new place for them. Could you rebase your PR, move the rules to https://github.com/microsoft/onnxscript/tree/main/onnxscript/rewriter/rules/common, and expose them in https://github.com/microsoft/onnxscript/blob/main/onnxscript/rewriter/rules/common/__init__.py? Thanks!

@justinchuby justinchuby removed their assignment Sep 5, 2025
@Stonesjtu Stonesjtu force-pushed the conv_and_hardswish_fusion branch from 9a28a08 to e8836d9 Compare September 6, 2025 14:20
@Stonesjtu
Copy link
Contributor Author

@justinchuby Done. I think the test files should also be moved out from the source code dir to tests/rules/something.py

@justinchuby
Copy link
Collaborator

@Stonesjtu
Copy link
Contributor Author

MHA attention fusion tests are failing, should be irrelevant to this PR.

affine_conv_fusion_rule,
conv_affine_fusion_rule,
)
from onnxscript.rewriter.rules.common._fuse_hardswish import fuse_hardswish_rules
Copy link
Collaborator

@justinchuby justinchuby Sep 10, 2025

Choose a reason for hiding this comment

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

Sorry I missed this: Could you expose the individual rules? I will merge this first - feel free to submit a follow up PR @Stonesjtu

@justinchuby justinchuby merged commit 821015a into microsoft:main Sep 10, 2025
29 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Development

Successfully merging this pull request may close these issues.

[Question] rewrite rules: merged into upstream or maintained out of tree

3 participants