Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1809273002:
Enable whole-program virtual function optimization in LTO mode. (Closed)
DescriptionEnable whole-program virtual function optimization in LTO mode.
Also move LTO config out of sanitizer config, and enable function
sections on all architectures (not just ARM) as it improves
the linker's ability to do ICF.
BUG=580389
Committed: https://crrev.com/5f4ec37b6548c11b0b470ce824a3d28f7f54fcd1
Cr-Commit-Position: refs/heads/master@{#385630}
Patch Set 1 #Patch Set 2 : Rename flag, remove is_cfi default, document desired end state; also enable function sections in al… #Patch Set 3 : Rename one more instance of is_lto #
Total comments: 5
Patch Set 4 : Address review comments #Patch Set 5 : Rebase #
Messages
Total messages: 35 (12 generated)
Description was changed from ========== Enable whole-program virtual function optimization in LTO mode. Also move LTO config out of sanitizer config. BUG=580389 ========== to ========== Enable whole-program virtual function optimization in LTO mode. Also move LTO config out of sanitizer config. BUG=580389 ==========
pcc@chromium.org changed reviewers: + brettw@chromium.org, thakis@chromium.org
Tangential comment: please, do not submit this CL until tomorrow. We should get our LTO Linux Perf buildbot after the upcoming (18:30) chromium.fyi master restart, and I would like to see it green (or red, and have a chance to fix it) before we turn this on. After we have it green, this CL will be in a perfect state to land.
The LTO Linux Perf buildbot is set up: https://build.chromium.org/p/chromium.fyi/builders/LTO Linux Perf This change is very welcome to src/
brettw: Ping.
A few points: - I commented on the patch where you added is_lto that it shouldn't be in sanitizers.gni. Also, I think the name isn't so great: "lto" is kind of obscure, and lto isn't something you "are", it's something you enable or use. - This flag is applied irrespective of optimization level. On Windows link time optimization is associated with optimization level. This makes more sense to me than defaulting link time code generation to whaever CFI is set to. (Does CFI make sense in debug builds?) - This flag controls only Posix linkers, but the flag is named generally, not inside a platform conditional, and not documented to be platform-specific. We have Windows link time optimization that's not controlled by this flag, and that behavior is that it applies only to targets with optimize_max applied. --- I think compiler/BUILD.gn is now the only place where you use this flag now. We should move is_lto to that build (doesn't have to be a .gni) and rename it something like enable_link_time_optimization. I would like to unify our policies here so this flag works uniformly. Maybe it should be called "allow_link_time_optimization" since the policy for applying it on Windows is a bit complicated, and then this flag would turn on that branch but not force it on for everybody. I also think the the flag should be a property of the optimize/optimize_max configs. I don't know what the right policy is for whether it should be on both or only _max, but keeping the optimization flags separate here is good for componentization and also allows targets to tweak their settings up or down as necessary.
On 2016/03/24 20:48:35, brettw wrote: > A few points: > > - I commented on the patch where you added is_lto that it shouldn't be in (That wasn't me :) ). But happy to move the flag. > sanitizers.gni. Also, I think the name isn't so great: "lto" is kind of obscure, > and lto isn't something you "are", it's something you enable or use. Makes sense. > - This flag is applied irrespective of optimization level. On Windows link time > optimization is associated with optimization level. This makes more sense to me > than defaulting link time code generation to whaever CFI is set to. (Does CFI > make sense in debug builds?) I guess Ivan made that the default because CFI requires LTO. But yes, it does seem like a strange default. > - This flag controls only Posix linkers, but the flag is named generally, not > inside a platform conditional, and not documented to be platform-specific. We > have Windows link time optimization that's not controlled by this flag, and that > behavior is that it applies only to targets with optimize_max applied. > > --- > > I think compiler/BUILD.gn is now the only place where you use this flag now. We > should move is_lto to that build (doesn't have to be a .gni) and rename it > something like enable_link_time_optimization. > > I would like to unify our policies here so this flag works uniformly. Maybe it > should be called "allow_link_time_optimization" since the policy for applying it > on Windows is a bit complicated, and then this flag would turn on that branch > but not force it on for everybody. > > I also think the the flag should be a property of the optimize/optimize_max > configs. I don't know what the right policy is for whether it should be on both > or only _max, but keeping the optimization flags separate here is good for > componentization and also allows targets to tweak their settings up or down as > necessary. Makes sense, but I'm not sure if enabling LTO should always be a property of a target config. The way I see it, I think the LTO situation for a target is a mixture of two roughly orthogonal things: 1) We've decided that for some particular target, we'd like to benefit from cross-TU inlining and other LTO features, so we'd like to turn on LTO for that target. 2) We've decided that we globally want to turn on a compiler feature that needs whole-program visibility, such as CFI or the whole-program vtable optimizer that this CL is about. In that case, we need to pass LTO flags to all targets. So maybe what we want is to have your "allow_link_time_optimization" on optimize_max to support the first use case, and for the second use case we would introduce temporary flags for those features, with the intention of removing them if/when the features land somewhere like an official build (then "is_official_build" would become the only user-visible knob). We already have such a flag for CFI -- "use_cfi". I propose to add another build flag -- "enable_whole_program_vtables" -- with the same policy. The condition for enabling the LTO flags would then be something like "allow_link_time_optimization || enable_whole_program_vtables || use_cfi".
On 2016/03/24 22:49:02, pcc1 wrote: > (That wasn't me :) ). But happy to move the flag. :) thanks > Makes sense, but I'm not sure if enabling LTO should always be a property of a > target config. The way I see it, I think the LTO situation for a target is a > mixture of two roughly orthogonal things: > > 1) We've decided that for some particular target, we'd like to benefit from > cross-TU inlining and other LTO features, so we'd like to turn on LTO for that > target. > > 2) We've decided that we globally want to turn on a compiler feature that needs > whole-program visibility, such as CFI or the whole-program vtable optimizer that > this CL is about. In that case, we need to pass LTO flags to all targets. > > So maybe what we want is to have your "allow_link_time_optimization" on > optimize_max to support the first use case, and for the second use case we would > introduce temporary flags for those features, with the intention of removing > them if/when the features land somewhere like an official build (then > "is_official_build" would become the only user-visible knob). We already have > such a flag for CFI -- "use_cfi". I propose to add another build flag -- > "enable_whole_program_vtables" -- with the same policy. The condition for > enabling the LTO flags would then be something like > "allow_link_time_optimization || enable_whole_program_vtables || use_cfi". Thinking about it more, unless we have a use-case for your #1, I'm OK not supporting the "per target LTO" feature. The state of the Windows thing being on optimize_max isn't so much based on design principle but rather it being slow and annoying so we only want it in extreme cases. It could be that on Posix we feel more comfortable enabling this more widely. So assuming we don't have a need for your use case #1, we can put these on the shared "optimized code" config so it will get applied to both "optimize" and "optimize_max" targets. We should just carefully document the difference in LTO meaning between Linux and Windows. I don't think we want "more global than optimize". In a release/official build, this will get applied to everything. I don't *think* anybody actually opts-out of this and uses debug optimization setup in a release build currently, and if they did in the future they would almost surely want to turn off LTO also. I don't know what all of the vtable/cfi stuff means. I do wonder whether we need so many build flags that are individually toggle-able. I have the impression that you guys are working on a project to enable this stuff and want to test it, but I don't necessarily see the case for individually changing these flags. Would a simple enable/disable switch for <whatever your project description is> be sufficient?
On 2016/03/28 22:42:46, brettw wrote: > On 2016/03/24 22:49:02, pcc1 wrote: > > (That wasn't me :) ). But happy to move the flag. > > :) thanks > > > Makes sense, but I'm not sure if enabling LTO should always be a property of a > > target config. The way I see it, I think the LTO situation for a target is a > > mixture of two roughly orthogonal things: > > > > 1) We've decided that for some particular target, we'd like to benefit from > > cross-TU inlining and other LTO features, so we'd like to turn on LTO for that > > target. > > > > 2) We've decided that we globally want to turn on a compiler feature that > needs > > whole-program visibility, such as CFI or the whole-program vtable optimizer > that > > this CL is about. In that case, we need to pass LTO flags to all targets. > > > > So maybe what we want is to have your "allow_link_time_optimization" on > > optimize_max to support the first use case, and for the second use case we > would > > introduce temporary flags for those features, with the intention of removing > > them if/when the features land somewhere like an official build (then > > "is_official_build" would become the only user-visible knob). We already have > > such a flag for CFI -- "use_cfi". I propose to add another build flag -- > > "enable_whole_program_vtables" -- with the same policy. The condition for > > enabling the LTO flags would then be something like > > "allow_link_time_optimization || enable_whole_program_vtables || use_cfi". > > Thinking about it more, unless we have a use-case for your #1, I'm OK not > supporting the "per target LTO" feature. The state of the Windows thing being on > optimize_max isn't so much based on design principle but rather it being slow > and annoying so we only want it in extreme cases. It could be that on Posix we > feel more comfortable enabling this more widely. SGTM. > So assuming we don't have a need for your use case #1, we can put these on the > shared "optimized code" config so it will get applied to both "optimize" and > "optimize_max" targets. We should just carefully document the difference in LTO > meaning between Linux and Windows. > > I don't think we want "more global than optimize". In a release/official build, > this will get applied to everything. I don't *think* anybody actually opts-out > of this and uses debug optimization setup in a release build currently, and if > they did in the future they would almost surely want to turn off LTO also. CFI is not an optimization, but a security feature, so I guess it doesn't entirely fit under "optimize", and it isn't inconceivable to want that feature in a debug build (e.g. in order to debug some problem with CFI). That said, I'd be ok with moving the flags to "optimize" for now and moving them elsewhere if we discover a compelling need. > I don't know what all of the vtable/cfi stuff means. I do wonder whether we need > so many build flags that are individually toggle-able. I have the impression > that you guys are working on a project to enable this stuff and want to test it, > but I don't necessarily see the case for individually changing these flags. > Would a simple enable/disable switch for <whatever your project description is> > be sufficient? The reason for the multiple flags is that the compiler features we have in flight are mostly orthogonal, and we'd like to be able to test them separately. We have 3 features in flight (vtable opt, CFI and the relative vtable ABI (crbug.com/580389)); I'm hoping that we won't actually need a flag for the last one as it should be easier to deploy than the other two. As I mentioned before, we plan to remove our flags aggressively as features land.
What about this: Move the flag in compiler/BUILD.gn and call it allow_posix_link_time_opt (or something) and use that in both of the optimize-on config variants, make sure to mention in the comments for the flag when it applies, and clearly document the desired end-state for this flag soup.
lgtm once Brett is happy with how this is spelled. Having the same spelling in gyp and gn is kinda nice.
Okay, I tried doing what you suggested, by moving the LTO config to a block that adds the flags to common_optimize_on_* conditional on an allow_posix_link_time_opt flag. Unfortunately, that didn't work -- some targets enable optimizations even in debug builds, which means that we end up with only part of the program being optimized with LTO. That presents a problem, because the specific LTO flags we're enabling are designed to work on a whole-program basis -- we may end up with miscompiles if the LTO flags are enabled on some translation units and not others. I then tried adding the LTO flags to both optimize and no_optimize if is_debug is disabled. That didn't work either, because nacl_bootstrap uses optimize, and the LTO flags are incompatible with it. What I ended up with is the CL I've uploaded, which adds the flags to the compiler block, as I was doing before, but now conditional on is_debug. I don't know if you have any other suggestions on where to put the flags.
Description was changed from ========== Enable whole-program virtual function optimization in LTO mode. Also move LTO config out of sanitizer config. BUG=580389 ========== to ========== Enable whole-program virtual function optimization in LTO mode. Also move LTO config out of sanitizer config, and enable function sections on all architectures (not just ARM) as it improves the linker's ability to do ICF. BUG=580389 ==========
Thanks for trying those other things. Since there's a good reason why this can't go on the optimize config, it makes more sense to put it in the compiler one. I'm not super happy about the addition to toolchain.gni, but I don't have a better suggestion given that the flag needs to be used in the posix toolchain file in addition to the compiler config. Hopefully we can either make this always work automatically so this configuration isn't necessary! (Please don't forget to clean this up at such a time.) LGTM https://codereview.chromium.org/1809273002/diff/40001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/1809273002/diff/40001/build/config/compiler/B... build/config/compiler/BUILD.gn:398: # Allows the linker to apply ICF to the LTO object file. Also, when targeting 80 cols. https://codereview.chromium.org/1809273002/diff/40001/build/toolchain/win/BUI... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/1809273002/diff/40001/build/toolchain/win/BUI... build/toolchain/win/BUILD.gn:28: if (allow_posix_link_time_opt) { This seems super weird in the Windows toolchain file. It seems like this flag will always be false. I did a blame on this and I'm not convinced this check wasn't just an error that was added everywhere the concurrent link script was called. If that's your understanding too, let's remove the condition and always do the false arm of this block.
Thanks Brett. https://codereview.chromium.org/1809273002/diff/40001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/1809273002/diff/40001/build/config/compiler/B... build/config/compiler/BUILD.gn:398: # Allows the linker to apply ICF to the LTO object file. Also, when targeting On 2016/04/06 06:25:36, brettw wrote: > 80 cols. Done. https://codereview.chromium.org/1809273002/diff/40001/build/toolchain/win/BUI... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/1809273002/diff/40001/build/toolchain/win/BUI... build/toolchain/win/BUILD.gn:28: if (allow_posix_link_time_opt) { On 2016/04/06 06:25:36, brettw wrote: > This seems super weird in the Windows toolchain file. It seems like this flag > will always be false. I did a blame on this and I'm not convinced this check > wasn't just an error that was added everywhere the concurrent link script was > called. > > If that's your understanding too, let's remove the condition and always do the > false arm of this block. Agreed. We don't even have LLVM LTO stuff ported to GN, so this is just dead code at the moment.
The CQ bit was checked by pcc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1809273002/#ps60001 (title: "Address review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809273002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809273002/60001
https://codereview.chromium.org/1809273002/diff/40001/build/toolchain/win/BUI... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/1809273002/diff/40001/build/toolchain/win/BUI... build/toolchain/win/BUILD.gn:28: if (allow_posix_link_time_opt) { On 2016/04/06 21:42:01, pcc1 wrote: > On 2016/04/06 06:25:36, brettw wrote: > > This seems super weird in the Windows toolchain file. It seems like this flag > > will always be false. I did a blame on this and I'm not convinced this check > > wasn't just an error that was added everywhere the concurrent link script was > > called. > > > > If that's your understanding too, let's remove the condition and always do the > > false arm of this block. > > Agreed. We don't even have LLVM LTO stuff ported to GN, so this is just dead > code at the moment. (I mean LLVM LTO on Windows, of course.)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by pcc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1809273002/#ps80001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809273002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809273002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by pcc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809273002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809273002/80001
Message was sent while issue was closed.
Description was changed from ========== Enable whole-program virtual function optimization in LTO mode. Also move LTO config out of sanitizer config, and enable function sections on all architectures (not just ARM) as it improves the linker's ability to do ICF. BUG=580389 ========== to ========== Enable whole-program virtual function optimization in LTO mode. Also move LTO config out of sanitizer config, and enable function sections on all architectures (not just ARM) as it improves the linker's ability to do ICF. BUG=580389 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Enable whole-program virtual function optimization in LTO mode. Also move LTO config out of sanitizer config, and enable function sections on all architectures (not just ARM) as it improves the linker's ability to do ICF. BUG=580389 ========== to ========== Enable whole-program virtual function optimization in LTO mode. Also move LTO config out of sanitizer config, and enable function sections on all architectures (not just ARM) as it improves the linker's ability to do ICF. BUG=580389 Committed: https://crrev.com/5f4ec37b6548c11b0b470ce824a3d28f7f54fcd1 Cr-Commit-Position: refs/heads/master@{#385630} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5f4ec37b6548c11b0b470ce824a3d28f7f54fcd1 Cr-Commit-Position: refs/heads/master@{#385630}
Message was sent while issue was closed.
Do we ship LTO binaries to real users anywhere yet? Who would see this optimization today?
Message was sent while issue was closed.
No, we aren't shipping this yet. This is still blocked on (at least) infrastructure improvements and parallelism improvements in gn. krasin@ has been working on those things. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
