-
-
Notifications
You must be signed in to change notification settings - Fork 679
BREAKING CHANGE: Revise default optimization level #1776
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
|
Perhaps an agenda item for later today: Should the first bullet trigger a breaking change? If so, I'd probably split this. |
| "-O3z": { "value": { "optimizeLevel": 3, "shrinkLevel": 2 } } | ||
| "-O3z": { "value": { "optimizeLevel": 3, "shrinkLevel": 2 } }, | ||
| "-Ospeed": { "value": { "optimizeLevel": 3, "shrinkLevel": 0 } }, | ||
| "-Osize": { "value": { "optimizeLevel": 0, "shrinkLevel": 2, "converge": true } } |
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.
Hmm, does optimizeLevel: 0 really beter than optimizeLevel: 3 for size shrinking?
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.
Would assume it depends. If there is a lot that can be inlined for example, it would depend on how well it optimizes away afterwards. If it doesn't, not inlining in the first place may be better.
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 std/array test produces exactly the same binary size with both -O3z and -Oz. The bootstrapped compiler seems to have a difference of a couple bytes: 838.188 with -O3z vs 838.155 with -Oz.
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 see. Thanks for info!
torch2424
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.
Oh oops! I didn't realize I was a reviewer 😂
This looks great to me, as we discussed in the CG meeting. Again, thank you so much for this! 😄 🙏
|
Oh! And I think we discussed in the CG meeting, that we might want to make this PR breaking. Or, split out the non-breaking flag changes into another PR. And then make this one as a part of whatever the next breaking change is? 😄 |
Changes default optimization levels for
--optimizerespectively-OtooptimizeLevel=3andshrinkLevel=0(wasshrinkLevel=1), since-O3s, which was intended as a compromise, doesn't appear to be very useful in practice. Afterwards,-Ois equivalent to-O3respectively-Ospeed.fixes #1753