-
Notifications
You must be signed in to change notification settings - Fork 5
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
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.