New issue
Advanced search Search tips

Issue 598772 link

Starred by 7 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Feature


Sign in to add a comment

Make LTO work in Windows Clang builds

Project Member Reported by thakis@chromium.org, Mar 29 2016

Issue description

/GL tells cl.exe to write LTCG inputs, and link.exe /LTCG then consumes those, doing link-time optimizations.

We _could_ hook up /GL to -flto, but then clang-cl's outputs wouldn't work with link.exe (but they could work with lld-link.exe and it could be how we end up turning on LTO). It's probably confusing to hook up /GL to -flto though, as it makes clang-cl less "drop-in cl replacement". Probably clang-cl should just silently ignore that flag instead.

Decide on something and do it. Anyone have an opinion on what to do? (Not urgent.)
 

Comment 1 by thakis@chromium.org, Mar 29 2016

(spun off form  bug 504658 )

Comment 2 by r...@chromium.org, Mar 29 2016

I think we should expose -flto separately. I like our current behavior of warning that we ignored /GL. It's not an almost meaningless option like /fastfail or /ThrowingNew that we can easily discard.

Can we change Chromium to not pass the LTCG flags if clang==1?

Comment 3 by thakis@chromium.org, Mar 29 2016

Yes, that's what I'm doing for now (https://codereview.chromium.org/1828543003/) but I wasn't sure if that's the behavior we want going forward. But now that you mention it, it actually sounds like a pretty good approach.

If that's what we want to do: Once we have -flto and whatnot, the diag for /GL should probably say something like 'ignoring /GL; use -flto and lld-link.exe /whatever instead'
Project Member

Comment 4 by bugdroid1@chromium.org, Mar 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/de51a84b69b5b1dd458c65f6aaf2335f81b57785

commit de51a84b69b5b1dd458c65f6aaf2335f81b57785
Author: thakis <thakis@chromium.org>
Date: Wed Mar 30 12:30:11 2016

clang/win: Stop passing -Qunused-arguments.

All flags except /GS and /GL are accepted by clang-cl by now. To be able
to turn on -Qunused-arguments, don't pass /GS and /GL in clang builds
for now.

BUG= 504658 ,598772, 598767 
NOTRY=true

Review URL: https://codereview.chromium.org/1828543003

Cr-Commit-Position: refs/heads/master@{#383954}

[modify] https://crrev.com/de51a84b69b5b1dd458c65f6aaf2335f81b57785/build/common.gypi
[modify] https://crrev.com/de51a84b69b5b1dd458c65f6aaf2335f81b57785/build/config/compiler/BUILD.gn
[modify] https://crrev.com/de51a84b69b5b1dd458c65f6aaf2335f81b57785/build/config/win/BUILD.gn

Comment 5 Deleted

Comment 6 Deleted

Comment 7 Deleted

Comment 8 Deleted

Comment 9 Deleted

Comment 10 Deleted

Comment 11 Deleted

Comment 12 Deleted

Components: Build

Comment 15 by h...@chromium.org, Oct 4 2016

Blockedon: 652880

Comment 16 by h...@chromium.org, Oct 12 2016

Blockedon: 655125

Comment 17 by h...@chromium.org, Nov 2 2016

Cc: -h...@chromium.org thakis@chromium.org inglorion@chromium.org
Labels: clang OS-Windows
Owner: h...@chromium.org
Status: Started (was: Untriaged)
Summary: Make LTO work in Windows Clang builds (was: Figure out what to do with /GL flag in clang-cl)
Re-purposing this to track making LTO with clang-cl work.

Comment 18 by h...@chromium.org, Nov 2 2016

The bug I've been working on lately is:
http://llvm.org/PR30799 - Assertion "mapping to a source type"' failed
Now with a fix proposal: https://reviews.llvm.org/D26212

With that applied I can link test_ime_driver_library.dll (which I'm not sure why the chrome target depends on, but it does).

Next up: asserts while building chrome_child.dll.
Project Member

Comment 19 by bugdroid1@chromium.org, Nov 2 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3a6656ca882eb5b672719913af98d4f494de0be7

commit 3a6656ca882eb5b672719913af98d4f494de0be7
Author: hans <hans@chromium.org>
Date: Wed Nov 02 17:28:03 2016

Win/Clang: turn on LTO for Official builds that use LLD

BUG=598772

Review-Url: https://codereview.chromium.org/2390153003
Cr-Commit-Position: refs/heads/master@{#429320}

[modify] https://crrev.com/3a6656ca882eb5b672719913af98d4f494de0be7/build/config/compiler/BUILD.gn

Project Member

Comment 20 by bugdroid1@chromium.org, Nov 11 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fd6e90a2f9ee8552c8baeb9c581c6778f01075be

commit fd6e90a2f9ee8552c8baeb9c581c6778f01075be
Author: hans <hans@chromium.org>
Date: Fri Nov 11 19:21:14 2016

CrWinClangLLD bot: switch to official static builds to test LTO

After Chromium #429320, LTO is enabled for Official Win-Clang builds
that use the LLD linker.

This patch makes this buildbot exercise that configuration.

BUG=598772

Review-Url: https://codereview.chromium.org/2493893004
Cr-Commit-Position: refs/heads/master@{#431614}

[modify] https://crrev.com/fd6e90a2f9ee8552c8baeb9c581c6778f01075be/tools/mb/mb_config.pyl

Blockedon: 672229
Project Member

Comment 22 by bugdroid1@chromium.org, Feb 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3ba9c67e210df1f039f93548c1ad5bb2dfe3a945

commit 3ba9c67e210df1f039f93548c1ad5bb2dfe3a945
Author: hans <hans@chromium.org>
Date: Fri Feb 03 03:43:23 2017

Turn off LTO again for Official Win/Clang LLD builds

We're not working on this config at the moment, so having a red buildbot
testing it is just waste of cycles.

BUG= 688123 , 598772

Review-Url: https://codereview.chromium.org/2677593002
Cr-Commit-Position: refs/heads/master@{#447910}

[modify] https://crrev.com/3ba9c67e210df1f039f93548c1ad5bb2dfe3a945/build/config/compiler/BUILD.gn

Comment 23 by h...@chromium.org, Jul 20 2017

Cc: -inglorion@chromium.org h...@chromium.org
Owner: inglorion@chromium.org
Bob has been working on this lately.
Labels: Performance
Adding a label so this shows up in my weekly triage. JFYI - I won't be pushing or nagging like I might typically do (as a TPM), but I just want to keep an eye on progress.
(Most work for this will be in the LLVM repo, so you won't get bugdroid mail on this bug for most changes.)
So per the clang switchover thread, this work will help recover the regressions to NewTabPage.LoadTime and Startup.FirstWebContents.NonEmptyPaint2.ColdStartup UMA metrics  that came with the switchover to clang on Windows.

So when we have this working, we should check those metrics and see if they indeed recover (and maybe even improve more) as a result of this.
My planned next step here is to set up a Chromium bot that builds with ThinLTO. We can then choose to do an experiment similar to what we did for Clang vs. MSVC+PGO and see what ThinLTO does to UMA metrics. As it stands, I am not aware that we have any data on what ThinLTO or full LTO on Windows do to UMA metrics, although they do show good improvements in Speedometer.
Project Member

Comment 28 by bugdroid1@chromium.org, Sep 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/908df7cd424db006e1a068f1548cbf739d9aa142

commit 908df7cd424db006e1a068f1548cbf739d9aa142
Author: Peter Collingbourne <pcc@chromium.org>
Date: Fri Sep 15 21:16:22 2017

build: Adapt ThinLTO build configuration to Windows.

With this I can build a working chrome.exe with ThinLTO on Windows
using LLVM trunk plus a few under-review patches.

Bug: 598772
Change-Id: Ida40f0367e03326fe03d4880395103c12008b34a
Reviewed-on: https://chromium-review.googlesource.com/656262
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Hans Wennborg <hans@chromium.org>
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502378}
[modify] https://crrev.com/908df7cd424db006e1a068f1548cbf739d9aa142/build/config/compiler/BUILD.gn

Project Member

Comment 29 by bugdroid1@chromium.org, Sep 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/36aa926859274a7d8a019d3345e1934ca32fc025

commit 36aa926859274a7d8a019d3345e1934ca32fc025
Author: inglorion <inglorion@chromium.org>
Date: Fri Sep 15 22:36:05 2017

Add test configuration for ThinLTO Windows ToT bot

R=dpranke@chromium.org

Bug: 598772
Change-Id: If6ff2a07a57613d7fecc0b309d821a23e94e1ef2
Reviewed-on: https://chromium-review.googlesource.com/667960
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Bob Haarman <inglorion@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502410}
[modify] https://crrev.com/36aa926859274a7d8a019d3345e1934ca32fc025/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/36aa926859274a7d8a019d3345e1934ca32fc025/tools/mb/mb_config.pyl

Project Member

Comment 31 by bugdroid1@chromium.org, Sep 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/38c2d8bc854f1e76189bd676a25c935dc89cb07f

commit 38c2d8bc854f1e76189bd676a25c935dc89cb07f
Author: inglorion <inglorion@chromium.org>
Date: Tue Sep 19 21:11:52 2017

Add missing 'gclient_apply_config': ['chrome_internal'] to ClangToTWinThinLTO64

The bot is doing official builds, so needs the internal sources.

Bug: 598772
Change-Id: I06cde08e8fee65034546fb80a5d81ffa6d0917a7
Reviewed-on: https://chromium-review.googlesource.com/673546
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Hans Wennborg <hans@chromium.org>
Commit-Queue: Bob Haarman <inglorion@chromium.org>

[modify] https://crrev.com/38c2d8bc854f1e76189bd676a25c935dc89cb07f/scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py

Is this bug fixed? What's left?

Comment 33 by r...@chromium.org, Oct 25 2017

Not yet, we have a ThinLTO bot, but the last build went ~7 hours before something happened:
https://build.chromium.org/p/chromium.clang/builders/ToTWinThinLTO64/builds/72

The next build is in the works.

Comment 34 by h...@chromium.org, Dec 5 2017

Blockedon: 792131
Project Member

Comment 35 by bugdroid1@chromium.org, Jan 10 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/39b4dda89c4493a5b7848162dbcc8a5ac58ffee6

commit 39b4dda89c4493a5b7848162dbcc8a5ac58ffee6
Author: Takuto Ikuta <tikuta@google.com>
Date: Wed Jan 10 14:24:50 2018

Do not pass /LTCG for clang-cl build

Bug:  781680 , 598772
Change-Id: I5b746386516ddb153892a91b19e8c1689f09e94f
Reviewed-on: https://chromium-review.googlesource.com/859409
Commit-Queue: Takuto Ikuta <tikuta@google.com>
Commit-Queue: Nico Weber <thakis@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528305}
[modify] https://crrev.com/39b4dda89c4493a5b7848162dbcc8a5ac58ffee6/build/config/compiler/BUILD.gn

Labels: -Type-Bug Type-Feature
Blockedon: 818086
Blockedon: 786127
Blockedon: 856635
Blockedon: 871962
Blockedon: v8:8502
Blockedon: 920451

Sign in to add a comment