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

transforms: Add function-constant-pinning pass to specialize functions #2389

Merged
merged 12 commits into from
May 30, 2024

Conversation

AntonLydike
Copy link
Collaborator

@AntonLydike AntonLydike commented Mar 27, 2024

This adds the function-constant-pinning pass that takes annotated functions and creates specialized versions of them that pin a value to a constant.

Currently only supports pinning at the top level of a function. Also other limitations, that will be documented on a train ride soon!

See the filecheck for examples on how the pass works!

Copy link

codecov bot commented Mar 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.60%. Comparing base (4bfe7a3) to head (dc8000c).
Report is 50 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2389      +/-   ##
==========================================
- Coverage   89.79%   89.60%   -0.19%     
==========================================
  Files         354      359       +5     
  Lines       44579    45763    +1184     
  Branches     6684     6905     +221     
==========================================
+ Hits        40030    41008     +978     
- Misses       3574     3694     +120     
- Partials      975     1061      +86     

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

@PapyChacal PapyChacal added the transformations Changes or adds a transformatio label Mar 27, 2024
@AntonLydike AntonLydike changed the title transforms: Add function-specialization pass to specialize functions transforms: Add function-constant-pinning pass to specialize functions Apr 11, 2024
@AntonLydike AntonLydike force-pushed the anton/function-specialization branch from 906ba12 to 9537037 Compare May 6, 2024 14:35
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.

On one hand, the code looks ok. On the other hand, I would have preferred a slightly decomposed API. It feels like outlining the body should be separate to splitting the code on it being a constant. It feels like these could be separate Python helpers, with their own Python API, instead of a pass which seems quite specialized for a project. My understanding of MLIR and IREE is that this is a pattern that is encouraged. If it unblocks you, I think we should be ok to merge, but I'd be a tiny bit less concerned with the maintainability of this code if it were split into self-contained PRs, one of which provides and tests splitting IR based on a constant value, another one of which provides and tests outlining blocks to functions, and maybe another one that composes them into a pass.

@AntonLydike
Copy link
Collaborator Author

On one hand, the code looks ok. On the other hand, I would have preferred a slightly decomposed API. It feels like outlining the body should be separate to splitting the code on it being a constant. It feels like these could be separate Python helpers, with their own Python API, instead of a pass which seems quite specialized for a project. My understanding of MLIR and IREE is that this is a pattern that is encouraged. If it unblocks you, I think we should be ok to merge, but I'd be a tiny bit less concerned with the maintainability of this code if it were split into self-contained PRs, one of which provides and tests splitting IR based on a constant value, another one of which provides and tests outlining blocks to functions, and maybe another one that composes them into a pass.

I do understand your concerns here, I think this code will evolve over time, and I see a good chance that we yank it out again later in case we don't need it. Maybe we should put this in an experimental folder?

@superlopuh
Copy link
Member

Oh yeah experimental would make me less hesitant for sure, then YOLO ship it

@AntonLydike AntonLydike merged commit 6c01bab into main May 30, 2024
10 checks passed
@AntonLydike AntonLydike deleted the anton/function-specialization branch May 30, 2024 13:07
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