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

Function call CSE pass #45

Closed
wants to merge 3 commits into from
Closed

Conversation

arrangabriel
Copy link
Collaborator

Adds a pass that hoists repeated function calls with equivalent arguments in and between equations into new equations, so they are only computed once.

@arrangabriel
Copy link
Collaborator Author

It would be nice if you could take a look @mscuttari and see what you think.

As you asked it does not currently handle equations with induction variables.

Additionally I was unsure whether calls in the initial equations had to be handled differently, so for not they are ignored.

There is also just one test. I read the page on filecheck tests for mlir, and they mention that one should try to make the tests as isolated as possible. I tried to do that for the one I have made, so please let me know if something can be improved about that. I'll add some more before finishing the PR.

Copy link
Member

@mscuttari mscuttari left a comment

Choose a reason for hiding this comment

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

I've left some comments about possible code improvements. Some of them are mostly stylistic, others a little bit less.

lib/Dialect/BaseModelica/Transforms/CallCSE.cpp Outdated Show resolved Hide resolved
lib/Dialect/BaseModelica/Transforms/CallCSE.cpp Outdated Show resolved Hide resolved
lib/Dialect/BaseModelica/Transforms/CallCSE.cpp Outdated Show resolved Hide resolved
lib/Dialect/BaseModelica/Transforms/CallCSE.cpp Outdated Show resolved Hide resolved
lib/Dialect/BaseModelica/Transforms/CallCSE.cpp Outdated Show resolved Hide resolved
lib/Dialect/BaseModelica/Transforms/CallCSE.cpp Outdated Show resolved Hide resolved
lib/Dialect/BaseModelica/Transforms/CallCSE.cpp Outdated Show resolved Hide resolved
lib/Dialect/BaseModelica/Transforms/CallCSE.cpp Outdated Show resolved Hide resolved
@mscuttari mscuttari linked an issue Oct 14, 2024 that may be closed by this pull request
@arrangabriel arrangabriel force-pushed the arrangabriel/function-cse branch 5 times, most recently from 1990b4d to a7398f8 Compare October 17, 2024 16:22
lib/Dialect/BaseModelica/Transforms/CallCSE.cpp Outdated Show resolved Hide resolved
lib/Dialect/BaseModelica/Transforms/CallCSE.cpp Outdated Show resolved Hide resolved
lib/Dialect/BaseModelica/Transforms/CallCSE.cpp Outdated Show resolved Hide resolved
@arrangabriel arrangabriel force-pushed the arrangabriel/function-cse branch from aa3c4b2 to f9d8329 Compare October 21, 2024 10:54
@arrangabriel arrangabriel marked this pull request as ready for review October 21, 2024 10:55
@arrangabriel arrangabriel requested a review from a team as a code owner October 21, 2024 10:55
@arrangabriel arrangabriel force-pushed the arrangabriel/function-cse branch from 5a8761a to 1237c42 Compare October 24, 2024 17:04
Copy link
Member

@mscuttari mscuttari left a comment

Choose a reason for hiding this comment

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

LGTM. Perform the last fix and squash your commits into one ("Add calls CSE pass"). I will then merge.

lib/Dialect/BaseModelica/Transforms/CallCSE.cpp Outdated Show resolved Hide resolved
@arrangabriel arrangabriel force-pushed the arrangabriel/function-cse branch from 965fb28 to da0eba0 Compare October 25, 2024 11:37
Copy link
Member

@mscuttari mscuttari left a comment

Choose a reason for hiding this comment

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

Didn't some few more problems inside the code you modified. Hopefully this is all, but I will keep looking in the next days.
Also, please squash just your commits and keep mines untouched (take the "Draft CSE pass" at most).

@arrangabriel arrangabriel force-pushed the arrangabriel/function-cse branch 5 times, most recently from d7b4c41 to 90fb249 Compare October 28, 2024 10:32
@arrangabriel arrangabriel force-pushed the arrangabriel/function-cse branch from 90fb249 to d6211a3 Compare October 28, 2024 16:35
@arrangabriel arrangabriel force-pushed the arrangabriel/function-cse branch from d6211a3 to f628f89 Compare October 31, 2024 13:11
@arrangabriel arrangabriel force-pushed the arrangabriel/function-cse branch from f628f89 to 5aebc97 Compare October 31, 2024 13:12
@arrangabriel arrangabriel force-pushed the arrangabriel/function-cse branch from 5aebc97 to 407ecd0 Compare October 31, 2024 14:21
mscuttari
mscuttari previously approved these changes Oct 31, 2024
Copy link
Member

@mscuttari mscuttari left a comment

Choose a reason for hiding this comment

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

LGTM, merging.

@mscuttari
Copy link
Member

/fast-forward

@arrangabriel
Copy link
Collaborator Author

I made a few more changes, but nothing important

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSE to avoid repeated pure function calls with the same argument
2 participants