Skip to content
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

transformations: (lift-arith-to-linalg) Add generic FMA #3344

Closed
wants to merge 1 commit into from

Conversation

n-io
Copy link
Collaborator

@n-io n-io commented Oct 25, 2024

Adds the option to lift arith to generic fused multiply-add ops, with various flags to control behaviour.

@n-io n-io added the transformations Changes or adds a transformatio label Oct 25, 2024
@n-io n-io self-assigned this Oct 25, 2024
Copy link

codecov bot commented Oct 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.08%. Comparing base (a3e2e7c) to head (542da59).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3344   +/-   ##
=======================================
  Coverage   90.08%   90.08%           
=======================================
  Files         446      446           
  Lines       56419    56457   +38     
  Branches     5417     5421    +4     
=======================================
+ Hits        50824    50862   +38     
  Misses       4162     4162           
  Partials     1433     1433           

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

Copy link
Member

@superlopuh superlopuh left a comment

Choose a reason for hiding this comment

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

This feels like it's doing two things at the same time, and breaking the composability of rewrites. Could the ops be fused by a separate pass?

@n-io
Copy link
Collaborator Author

n-io commented Oct 25, 2024

The behaviour can be switched on and off by a flag, but doing this in a separate pass is (mildly) more complicated, as the regions contain arith.addf and arith.mulf which the pass as-is would attempt to translate.

@n-io n-io requested a review from superlopuh October 25, 2024 19:22
@AntonLydike
Copy link
Collaborator

I agree with Sasha here, lifting arith to linalg is one thing, but fusing operations together is a different thing entirely

@n-io
Copy link
Collaborator Author

n-io commented Oct 28, 2024

Closing in favour of doing this in a separate pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
transformations Changes or adds a transformatio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants