Enable WPO for the x64 Official builds.

Enable WPO for the x64 Official builds.

BUG=490934
Committed: https://crrev.com/ffb89045a543e91c09f2cf2c168a823e7b943f27
Cr-Commit-Position: refs/heads/master@{#397460}

[Sébastien Marchand, 2016-05-28 16:19:47]

Description was changed from

==========
Enable WPO for the x64 Official builds.

BUG=490934
==========

to

==========
Enable WPO for the x64 Official builds.

DO NOT LAND until the fix in Brotli has been fixed.

I've fixed the uninitialized value error in the Brotli repo and made a pull
request for it (https://github.com/google/brotli/pull/357) , I'm just leaving it
here for now until my PR has been accepted and merged into Chromium.

BUG=490934
==========

[Sébastien Marchand, 2016-05-28 16:19:47]

sebmarchand@chromium.org changed reviewers:
+ brucedawson@chromium.org, laforge@chromium.org, scottmg@chromium.org

[Sébastien Marchand, 2016-06-01 16:44:38]

Description was changed from

==========
Enable WPO for the x64 Official builds.

DO NOT LAND until the fix in Brotli has been fixed.

I've fixed the uninitialized value error in the Brotli repo and made a pull
request for it (https://github.com/google/brotli/pull/357) , I'm just leaving it
here for now until my PR has been accepted and merged into Chromium.

BUG=490934
==========

to

==========
Enable WPO for the x64 Official builds.


BUG=490934
==========

[Sébastien Marchand, 2016-06-01 16:44:46]

PTAL.

[Sébastien Marchand, 2016-06-01 16:54:24]

+ some Chrome-Windows folks

[scottmg, 2016-06-01 16:58:14]

lgtm

[brucedawson, 2016-06-01 17:17:39]

lgtm

[Will Harris, 2016-06-01 17:19:52]

I don't think you need my lgtm but you can have it anyway.

[grt (UTC plus 2), 2016-06-01 18:46:59]

https://codereview.chromium.org/2018293002/diff/100001/build/config/compiler/...
File build/config/compiler/compiler.gni (right):

https://codereview.chromium.org/2018293002/diff/100001/build/config/compiler/...
build/config/compiler/compiler.gni:33: # Whether or not the official builds
should be build with full WPO. Enabled by
build -> built

https://codereview.chromium.org/2018293002/diff/100001/chrome/installer/mini_...
File chrome/installer/mini_installer/BUILD.gn (right):

https://codereview.chromium.org/2018293002/diff/100001/chrome/installer/mini_...
chrome/installer/mini_installer/BUILD.gn:170:
"//build/config/compiler:default_optimization",
i don't get this. why?

[Sébastien Marchand, 2016-06-01 19:16:31]

https://codereview.chromium.org/2018293002/diff/100001/build/config/compiler/...
File build/config/compiler/compiler.gni (right):

https://codereview.chromium.org/2018293002/diff/100001/build/config/compiler/...
build/config/compiler/compiler.gni:33: # Whether or not the official builds
should be build with full WPO. Enabled by
On 2016/06/01 18:46:59, grt (slow) wrote:
> build -> built

Done. Thanks :)

https://codereview.chromium.org/2018293002/diff/100001/chrome/installer/mini_...
File chrome/installer/mini_installer/BUILD.gn (right):

https://codereview.chromium.org/2018293002/diff/100001/chrome/installer/mini_...
chrome/installer/mini_installer/BUILD.gn:170:
"//build/config/compiler:default_optimization",
On 2016/06/01 18:46:59, grt (slow) wrote:
> i don't get this. why?

It's because the default optimization now adds the /GL compiler flag for
Official x64, which doesn't work with the mini_installer. I haven't added a
comment because I thought that the general comment for this group was clear
enough ?

The alternative would be to depend on the 'optimize_no_wpo' config like we do in
ffmpeg:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...

Which solution do you prefer ?

[brucedawson, 2016-06-01 19:25:22]

> The alternative would be to depend on the 'optimize_no_wpo' config like we do
in
> ffmpeg:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...
> 
> Which solution do you prefer ?

optimize_no_wpo seems more obvious. Explicitly opting in to default_optimization
seems... weird.

[Sébastien Marchand, 2016-06-01 19:39:44]

Alright, switched to the optimize_no_wpo config.

[grt (UTC plus 2), 2016-06-01 19:56:14]

grt@chromium.org changed reviewers:
+ grt@chromium.org

[grt (UTC plus 2), 2016-06-01 19:56:15]

lgtm

[Sébastien Marchand, 2016-06-02 17:40:06]

The CQ bit was checked by sebmarchand@chromium.org

[Sébastien Marchand, 2016-06-02 17:40:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
scottmg@chromium.org, brucedawson@chromium.org, wfh@chromium.org,
grt@chromium.org
Link to the patchset: https://codereview.chromium.org/2018293002/#ps160001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-06-02 17:40:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2018293002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2018293002/160001

[commit-bot: I haz the power, 2016-06-02 17:46:19]

Description was changed from

==========
Enable WPO for the x64 Official builds.


BUG=490934
==========

to

==========
Enable WPO for the x64 Official builds.


BUG=490934
==========

[commit-bot: I haz the power, 2016-06-02 17:46:21]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-06-02 17:49:08]

Description was changed from

==========
Enable WPO for the x64 Official builds.


BUG=490934
==========

to

==========
Enable WPO for the x64 Official builds.

BUG=490934

Committed: https://crrev.com/ffb89045a543e91c09f2cf2c168a823e7b943f27
Cr-Commit-Position: refs/heads/master@{#397460}
==========

[commit-bot: I haz the power, 2016-06-02 17:49:10]

Patchset 9 (id:??) landed as
https://crrev.com/ffb89045a543e91c09f2cf2c168a823e7b943f27
Cr-Commit-Position: refs/heads/master@{#397460}

[Nico, 2016-06-02 19:32:51]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-06-02 19:32:52]

this breaks clang-cl builds, "clang-cl.exe: error: argument unused during
compilation: '/GL'"

You need something like
https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&...
in the other places that set WholeProgramOptimization to true.

From what I can tell, gn needs something like
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/compi...
aroundhttps://code.google.com/p/chromium/codesearch#chromium/src/build/config/compiler/BUILD.gn&l=1257
too.

[Sébastien Marchand, 2016-06-02 19:38:01]

Fixing this right now.

[brucedawson, 2016-06-03 00:47:53]

https://codereview.chromium.org/2018293002/diff/100001/chrome/installer/mini_...
File chrome/installer/mini_installer/BUILD.gn (right):

https://codereview.chromium.org/2018293002/diff/100001/chrome/installer/mini_...
chrome/installer/mini_installer/BUILD.gn:170:
"//build/config/compiler:default_optimization",
Well, as sometimes happens the reviewers successfully convinced the CL author to
do the wrong thing. I think that default_optimization was correct. When building
a debug version of the mini_installer today I hit this warning:

[4 processes, 6357/6386 @ 4.8/s : 1322.877s ] LINK mini_installer.exe
LINK : warning LNK4075: ignoring '/INCREMENTAL' due to '/OPT:ICF' specification

Do we need a default_optimization_no_wpo config?

[Sébastien Marchand, 2016-06-03 01:13:18]

https://codereview.chromium.org/2018293002/diff/100001/chrome/installer/mini_...
File chrome/installer/mini_installer/BUILD.gn (right):

https://codereview.chromium.org/2018293002/diff/100001/chrome/installer/mini_...
chrome/installer/mini_installer/BUILD.gn:170:
"//build/config/compiler:default_optimization",
On 2016/06/03 00:47:53, brucedawson wrote:
> Well, as sometimes happens the reviewers successfully convinced the CL author
to
> do the wrong thing. I think that default_optimization was correct. When
building
> a debug version of the mini_installer today I hit this warning:
> 
> [4 processes, 6357/6386 @ 4.8/s : 1322.877s ] LINK mini_installer.exe
> LINK : warning LNK4075: ignoring '/INCREMENTAL' due to '/OPT:ICF'
specification
> 
> Do we need a default_optimization_no_wpo config?

I think that this will add too much complexity to our build system, I'd suggest
to simply put the '//build/config/compiler:optimize_no_wpo' config behind a 'if
(!is_debug)' flag ? (in this file). This will be closer to what's being done in
the default_optimization config:
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/compi...

[brucedawson, 2016-06-03 01:28:05]

> I think that this will add too much complexity to our build system, I'd
suggest
> to simply put the '//build/config/compiler:optimize_no_wpo' config behind a
'if
> (!is_debug)' flag ? (in this file). This will be closer to what's being done
in
> the default_optimization config:
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/compi...

Sounds good. Further discussion should be on a new bug or CL.