-
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
base: master
Are you sure you want to change the base?
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.
for (EquationInstanceOp equationOp : dynamicEquationOps) { | ||
EquationTemplateOp templateOp = equationOp.getTemplate(); | ||
if (!templateOp.getInductionVariables().empty() || | ||
llvm::is_contained(templateOps, templateOp)) { |
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.
This is O(n^2). It would be better to check for already visited templates by using a set, while at the same time keeping the vector for deterministic output.
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.
Yeah, I'll fix it
965fb28
to
da0eba0
Compare
Adds a pass that hoists repeated function calls with equivalent arguments in and between equations into new equations, so they are only computed once.