-
Notifications
You must be signed in to change notification settings - Fork 9
Function call CSE pass #45
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
Conversation
|
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. |
There was a problem hiding this 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.
1990b4d to
a7398f8
Compare
aa3c4b2 to
f9d8329
Compare
5a8761a to
1237c42
Compare
There was a problem hiding this 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.
965fb28 to
da0eba0
Compare
There was a problem hiding this 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).
lib/Dialect/BaseModelica/Transforms/EquationExpressionOpInterfaceImpl.cpp
Outdated
Show resolved
Hide resolved
lib/Dialect/BaseModelica/Transforms/EquationExpressionOpInterfaceImpl.cpp
Outdated
Show resolved
Hide resolved
lib/Dialect/BaseModelica/Transforms/EquationExpressionOpInterfaceImpl.cpp
Outdated
Show resolved
Hide resolved
lib/Dialect/BaseModelica/Transforms/EquationExpressionOpInterfaceImpl.cpp
Outdated
Show resolved
Hide resolved
lib/Dialect/BaseModelica/Transforms/EquationExpressionOpInterfaceImpl.cpp
Outdated
Show resolved
Hide resolved
d7b4c41 to
90fb249
Compare
lib/Dialect/BaseModelica/Transforms/EquationExpressionOpInterfaceImpl.cpp
Outdated
Show resolved
Hide resolved
90fb249 to
d6211a3
Compare
lib/Dialect/BaseModelica/Transforms/EquationExpressionOpInterfaceImpl.cpp
Outdated
Show resolved
Hide resolved
d6211a3 to
f628f89
Compare
f628f89 to
5aebc97
Compare
lib/Dialect/BaseModelica/Transforms/EquationExpressionOpInterfaceImpl.cpp
Outdated
Show resolved
Hide resolved
lib/Dialect/BaseModelica/Transforms/EquationExpressionOpInterfaceImpl.cpp
Outdated
Show resolved
Hide resolved
5aebc97 to
407ecd0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, merging.
|
/fast-forward |
407ecd0 to
6f004f5
Compare
|
I made a few more changes, but nothing important |
Adds a pass that hoists repeated function calls with equivalent arguments in and between equations into new equations, so they are only computed once.