Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2390153003:
Win/Clang: turn on LTO for Official builds that use LLD (Closed)
DescriptionWin/Clang: turn on LTO for Official builds that use LLD
BUG=598772
Committed: https://crrev.com/3a6656ca882eb5b672719913af98d4f494de0be7
Cr-Commit-Position: refs/heads/master@{#429320}
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebase #Messages
Total messages: 22 (6 generated)
hans@chromium.org changed reviewers: + rnk@chromium.org, thakis@chromium.org
After Clang r283258, this should Just Work(tm). What do you think?
https://codereview.chromium.org/2390153003/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2390153003/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:1412: "/GL", # Whole program optimization. Should we alias /GL to -flto in clang, or do you think that would be confusing because it doesn't actually interoperate with link.exe /LTCG?
https://codereview.chromium.org/2390153003/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2390153003/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:1412: "/GL", # Whole program optimization. On 2016/10/04 22:25:40, Reid Kleckner wrote: > Should we alias /GL to -flto in clang, or do you think that would be confusing > because it doesn't actually interoperate with link.exe /LTCG? Yes, I think that might be confusing. We discussed this a little already in https://bugs.chromium.org/p/chromium/issues/detail?id=598772#c2
lgtm
The Clang roll I was waiting for has been in for a while now, so let's land this.
The Clang roll I was waiting for has been in for a while now, so let's land this.
The Clang roll I was waiting for has been in for a while now, so let's land this.
The CQ bit was checked by hans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rnk@chromium.org Link to the patchset: https://codereview.chromium.org/2390153003/#ps20001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm Please watch the build time graph and report the impact on build time here. As said before, I'd very prefer if we could get things fast enough without lto (which would also help all the other platforms).
Looks like you said on the bug that things still assert when building chrome_child.dll – maybe this shouldn't go in if that's still the case? (Or I misunderstood the comment on the bug) On Wed, Nov 2, 2016 at 12:28 PM, <thakis@chromium.org> wrote: > lgtm > > Please watch the build time graph and report the impact on build time here. > > As said before, I'd very prefer if we could get things fast enough without > lto > (which would also help all the other platforms). > > https://codereview.chromium.org/2390153003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/11/02 16:29:20, Nico (mostly afk Wed-Fri) wrote: > Looks like you said on the bug that things still assert when building > chrome_child.dll – maybe this shouldn't go in if that's still the case? (Or > I misunderstood the comment on the bug) Sorry, that's only when I turn it up a notch and use "-flto" on all the translation units (full_wpo_on_official). The names are really confusing here. I guess on Windows we have "regular LTO" and "full LTO", and this only turns on the former. The latter is what we use for shipping MSVC builds though, and I'd like to make it work and see what performance we'd get from that with Clang. > On Wed, Nov 2, 2016 at 12:28 PM, <mailto:thakis@chromium.org> wrote: > > > lgtm > > > > Please watch the build time graph and report the impact on build time here. > > > > As said before, I'd very prefer if we could get things fast enough without > > lto > > (which would also help all the other platforms). I agree. Getting this on the lld buildbots is mostly to make sure we don't regress our ability here.
The CQ bit was checked by hans@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
On 2016/11/02 16:29:20, Nico (mostly afk Wed-Fri) wrote: > Looks like you said on the bug that things still assert when building > chrome_child.dll – maybe this shouldn't go in if that's still the case? (Or > I misunderstood the comment on the bug) This also only affects official builds that use LLD, so it will only affect ~3 bots on our waterfall. We might want to add some non-official LLD builders to maintain coverage of the non-LTO link.
Message was sent while issue was closed.
Description was changed from ========== Win/Clang: turn on LTO for Official builds that use LLD BUG=598772 ========== to ========== Win/Clang: turn on LTO for Official builds that use LLD BUG=598772 Committed: https://crrev.com/3a6656ca882eb5b672719913af98d4f494de0be7 Cr-Commit-Position: refs/heads/master@{#429320} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/3a6656ca882eb5b672719913af98d4f494de0be7 Cr-Commit-Position: refs/heads/master@{#429320} |
