-
Notifications
You must be signed in to change notification settings - Fork 6.2k
Use side effects of user-defined functions in other optimizer steps. #12091
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
I think it might really make sense to cache the side-effects in the context (same is true for the other side-effects as well). |
dbb304f
to
c33fd24
Compare
aa07c7f
to
c2147b8
Compare
858bc30
to
b0f9acf
Compare
c2147b8
to
a904964
Compare
Performance does not seem to be as big an issue as I thought. |
95b68f0
to
2c2269d
Compare
a904964
to
3149036
Compare
Fuzzer update: There is a stack-use-after-scope here:
(see #10973 (comment) for discussion on a similar issue) I am not sure if YulString as an rvalue reference temporary is the issue. At least one of similar issues in the past could be fixed by replacing temporary with a value on the stack. (see #11371) |
3149036
to
785e16f
Compare
Before merging this, some tests may need to be modified so that they are still purposeful. |
288de24
to
c64e7d6
Compare
Changed the tests to be meaningful, please double-check. |
I think this could need some more tests in the relevant optimizer steps. |
8683dfb
to
86294c4
Compare
245c277
to
be6fb5f
Compare
holds_alternative<ExpressionStatement>(_statement) && | ||
isTerminatingBuiltin(std::get<ExpressionStatement>(_statement)) | ||
containsNonContinuingFunctionCall(std::get<ExpressionStatement>(_statement).expression) | ||
) | ||
return ControlFlow::Terminate; |
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.
Not that we need it right away, but in general it makes sense to distinguish between terminating and reverting here, doesn't it?
function f() { return(0,0) }
function g() { revert(0,0) }
{
switch calldataload(0)
case 0 {
sstore(0, 0) // needs to stay
f()
}
case 1 {
sstore(0, 0) // can be removed
g()
}
}
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.
Assuming that the side effects are correct :-), this looks good!
I'm wondering if it's worth to catch this eventually:
for {} 1 {} {
if g() { return(...) }
}
sstore(0, 1) // can be removed
but no need now in any case.
No description provided.