-
Notifications
You must be signed in to change notification settings - Fork 6.3k
Do not optimize IR without optimized outputs #15162
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
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.
Note that these tests do not really test much. I added them so that this unintuitive option combination at least gets used, but the change is not really visible in compiler output. We won't detect anything unless it just crashes.
The difference is only visible in execution time. Both of these should now finish almost instantly:
time solc test/benchmarks/chains.sol --ir --optimizetime solc test/benchmarks/chains.sol --ir --optimize --via-ir| @@ -0,0 +1 @@ | |||
| --ir --optimize --debug-info none | |||
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.
Can I have another one with --ir-optimized --optimize --debug-info none, so that we can at least see the difference between outputs in this 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.
Added. Only for this case though because I don't think such a test is overly useful. --ir-optimized --optimize is the standard thing to do and the test is not checking its correctness anyway.
49f1847 to
7565c52
Compare
|
This pull request is stale because it has been open for 14 days with no activity. |
7565c52 to
5a232b8
Compare
clonker
left a comment
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.
At first I was tripping over the _optimized argument in generateIR which also set to true if viaIR and generateEvmBytecode were active even if the goal wasn't set to Optimized until it occurred to me that eg stack allocations would still be optimized even if no optimize cli option was given. I think it might have been clearer to me if the argument was called _runOptimizer instead, which is also closer to the naming in the optimizer settings struct. Still, I think it's fine as it is.
5a232b8 to
e40b184
Compare
|
@clonker I think that the confusing thing is actually that the "optimized IR" output does not necessarily contain optimized IR :) It depends on the optimizer settings. It's just the main IR output that's used for compilation via IR and needing it for bytecode generation is natural, regardless of the optimization level. This is tripping people up all the time, especially on the CLI (@aarlt recently complained about that :)). But I do think that things could be clearer. I wasn't fully satisfied with the initial version. I tried something new now - I replaced |
clonker
left a comment
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.
You hit the nail on the head. That was the root of my confusion:) I like this version much better.
e40b184 to
79e2bfc
Compare
|
Rebased to resolve a conflict with #15228. I also tweaked it a little more for readability:
|
79e2bfc to
1a60d18
Compare
Fixes #15062.