-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Fix inlining order to correspond to source order. #9672
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
@@ -36,20 +36,15 @@ | |||
// function f(x) |
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.
The order functions are processed changed here: Now, first g
is inlined into f
. Then we take a look at g
and do not inline the call to f
anymore because it is directly recursive now.
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.
Is the comment at the beginning of this file still relevant?
If it is, there is a typo there statemenst
, might wanna fix that in this PR if the comment stays.
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 think it is still relevant.
219bc74
to
e72afcb
Compare
I'll try adding a yul-only test. |
Couldn't really come up with a test, but the change in the expectations at least show that this change has an effect. |
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.
It looks good, but I'm still wondering whether there would be an even more resilient way to handle this.
@@ -36,20 +36,15 @@ | |||
// function f(x) |
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.
Is the comment at the beginning of this file still relevant?
If it is, there is a typo there statemenst
, might wanna fix that in this PR if the comment stays.
@@ -80,10 +80,20 @@ void FullInliner::run() | |||
|
|||
// TODO it might be good to determine a visiting order: |
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.
Isn't this +- what this PR does now?
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 PR makes it less chaotic, but the TODO suggests that we should find an order that results in better code.
Fixes #9573
No tests are added because I was not able to reproduce the reported issue in 0.7.0.