1341525 - Support PGO in clang-cl builds

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(Away)]

I managed to PGO a toy program on Windows using clang-cl.exe and lld-link.exe built from LLVM trunk. Doing this for libxul would of course be much more challenging, but I think it's worth investigating. Someday this may allow us to do official builds with clang-cl.

It's possible that this work may also fix bug 1064049 (clang PGO on Linux) as a byproduct.

Comment 1

[(Away)]

Attachment: Toy program showing build steps

Comment 2

[(no longer active)]

Does this really depend on bug 1384434?  AFAIK a project that can be compiled with clang-cl should be able to use PGO without lld, see <https://clang.llvm.org/docs/UsersManual.html#profile-guided-optimization>...

Comment 3

[(Away)]

What other linker would you use? The documentation is not especially clear about this.

Comment 4

[Mike Hommey [:glandium]]

Presumably, clang-cl pgo doesn't work at link time like MSVC's (assuming it works like clang pgo)

Comment 5

[(no longer active)]

MS link.exe.  clang PGO compiler doesn't use link-time code generation as far as I can tell.  It uses the profile information when doing the second round of building to perform PGO optimizations.

There is also LTO (link-time optimizations) which requires running a linker plugin to do link-time code generation, and if we want to use that we need to build with lld.  To the best of my knowledge you can use LTO and PGO either together or independently.

The MSVC's PGO compiler is weird in the sense that its PGO compiler requires LTCG as a dependency.  I never really realized why that is...

Comment 6

[(Away)]

It never occurred to me that one could do PGO in any other way, because, well, link.exe is what I grew up with.

TIL. Thanks!

Comment 7

[(Away)]

I confirmed that I can PGO the toy program with clang-cl and the MS linker -- just replace "lld-link.exe" with "link.exe" in the instructions. It seems like this will make things much easier!

Please correct me where I'm wrong, but I think what we need is:

- During MOZ_PROFILE_GENERATE:
  - Add "-fprofile-instr-generate" to compiler flags
  - Add "clang_rt.profile-$ARCH.lib" to linker libs

- During maybe_clobber_profiledbuild:
  - Do this phase in the gcc/clang way (as in actually clobber)
  - Call "llvm-profdata merge -o default.profdata default.profraw"  (*)

- During MOZ_PROFILE_USE:
  - Add "-fprofile-instr-use" to compiler flags

(*) Or possibly something a little more fancy than this, depending on what happens with multiple binaries or multiple runs

Comment 8

[(Away)]

In attempting to do this on the our tree:
> - During MOZ_PROFILE_GENERATE:
>   - Add "-fprofile-instr-generate" to compiler flags
>   - Add "clang_rt.profile-$ARCH.lib" to linker libs

I get a bunch of multiply-defined symbol errors as illustrated by the reduced case below. These __profd symbols appear multiple times in a way that link.exe can't handle, but lld-link.exe can. At first glance I can't tell if this is an LLVM bug or just a consequence of the two linkers handling things in different ways (are these ELF-style weak symbols, perhaps?).

$ cat one.cpp
#include <stdio.h>

void SayHello();

int main()
{
  SayHello();
  printf("world");
  return 0;
}

$ cat two.cpp
#include <stdio.h>

void SayHello()
{
  printf("hello ");
}

$ clang-cl -c -fprofile-instr-generate one.cpp
$ clang-cl -c -fprofile-instr-generate two.cpp
$ link one.obj two.obj clang_rt.profile-x86_64.lib
Microsoft (R) Incremental Linker Version 14.00.24215.1
Copyright (C) Microsoft Corporation.  All rights reserved.

two.obj : error LNK2005: __profd_printf already defined in one.obj
two.obj : error LNK2005: __profd__vfprintf_l already defined in one.obj
two.obj : error LNK2005: __profd___local_stdio_printf_options already defined in one.obj
one.exe : fatal error LNK1169: one or more multiply defined symbols found

$ lld-link one.obj two.obj clang_rt.profile-x86_64.lib
$ one.exe
hello world

Comment 9

[(Away)]

> These __profd symbols appear multiple times in a way that link.exe can't handle, but lld-link.exe can.

lld-link is ok with the toy program, but when I try it on an actual build of m-c, it too hits:

lld-link.exe: error: duplicate symbol: __profc_?width@ios_base@std@@QEBA_JXZ in Unified_cpp_minidump-analyzer0.obj and in ..\google-breakpad\src\processor\Unified_cpp_src_processor0.obj

lld-link.exe: error: duplicate symbol: __profc_?flags@ios_base@std@@QEBAHXZ in Unified_cpp_minidump-analyzer0.obj and in ..\google-breakpad\src\processor\Unified_cpp_src_processor0.obj

and many more. Interestingly they're all 'profc' rather than 'profd' this time.

Comment 10

[(no longer active)]

For the record, the "__profd_" and "__profc_" prefixes come from this code in LLVM: https://github.com/llvm-mirror/llvm/blob/b8bd144ca177b887e88683e1081842071bef0889/include/llvm/ProfileData/InstrProf.h#L85

Comment 11

[(Away)]

I finally managed to run this through creduce. Filed as https://bugs.llvm.org/show_bug.cgi?id=38251

Comment 12

[(Away)]

Attachment: Enable PGO in 64-bit clang-cl builds

32-bit is blocked by bug 1479800 that I don't want to wait for.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2011dae40293
Enable PGO in 64-bit clang-cl builds. r=froydnj

Comment 14

[Andreea Pavel [:apavel]]

Backed out for failing win Sa builds

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2011dae40293da2aceba942e8c7d95d0a4dfffbf

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=191463113&repo=mozilla-inbound&lineNumber=1273

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/b2b6bfdafb1d6b9fa8f5233ebe6025d8c60a1d09

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a3882f27aa3
Enable PGO in 64-bit clang-cl builds. r=froydnj

Comment 16

[Andreea Pavel [:apavel]]

Backed out for failing win Sa builds

Push that started the failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7a3882f27aa3ff5e4f2a2f76da1469f8234c47ab

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=191468617&repo=mozilla-inbound&lineNumber=1293

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/c445479df5c8884109728d19112da0afdde51189

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/084b1e903489
Enable PGO in 64-bit clang-cl builds. r=froydnj

Comment 18

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/084b1e903489

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 8996716 [details] [diff] [review]
Enable PGO in 64-bit clang-cl builds

Review of attachment 8996716 [details] [diff] [review]:
-----------------------------------------------------------------

::: Makefile.in
@@ +307,5 @@
>  endif
>  else
>  ifdef CLANG_CL
>  maybe_clobber_profiledbuild: clean
> +	$(LLVM_PROFDATA) merge -o $(DEPTH)/merged.profdata $(DEPTH)/*.profraw

Shouldn't we have one per binary produced? Or is clang happy that the merged profile data contains things from unrelated objects?

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 8996716 [details] [diff] [review]
Enable PGO in 64-bit clang-cl builds

Review of attachment 8996716 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/moz.configure/toolchain.configure
@@ +1306,5 @@
> +#                           bool(pgo) and cc.type == 'clang-cl'))
> +llvm_profdata = check_prog('LLVM_PROFDATA', ['llvm-profdata'],
> +                           allow_missing=True)
> +
> +add_old_configure_assignment('LLVM_PROFDATA', llvm_profdata)

This is not necessary.

Comment 21

[Ionuț Goldan [:igoldan]]

Noticed some new perf improvements on Windows:

== Change summary for alert #14709 (as of Thu, 02 Aug 2018 10:15:45 GMT) ==

Improvements:

  5%  ts_paint windows10-64 pgo e10s stylo     313.75 -> 298.75
  5%  sessionrestore windows10-64 pgo e10s stylo222.67 -> 212.31
  5%  sessionrestore_no_auto_restore windows10-64 pgo e10s stylo257.83 -> 246.17

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14709

Comment 22

[(Away)]

In addition to making this faster this also made the installer 2.8MB/4% smaller.

Comment 23

[(Away)]

Additional improvements from bug 1480708 comment 3:

== Change summary for alert #14707 (as of Thu, 02 Aug 2018 09:39:51 GMT) ==

Improvements:

  8%  speedometer windows10-64 pgo e10s stylo     75.33 -> 81.01
  7%  damp windows10-64 pgo e10s stylo            246.51 -> 229.42
  6%  tp6_google windows10-64 pgo e10s stylo      382.90 -> 360.42
  6%  a11yr windows10-64 pgo e10s stylo           166.40 -> 157.13
  6%  tp6_amazon windows10-64 pgo e10s stylo      215.27 -> 203.42

Comment 24

[koboltzz]

I don't know if I'm violating any (dev) behavioral rules, but as someone who is compiling from source, I wanted to note that I always get the error 

lld-link.exe: error: could not open clang_rt.profile-x86_64.lib: no such file or directory

when I try to compile clang PGO build for 'inbound' firefox x64.
Did I misunderstand, that it should work now?

Comment 25

[(Away)]

Thanks for reporting. I've filed bug 1483524 to make the build system handle this automatically, but I probably won't have time to work on it soon. In the meantime you'll need to set $LIB to include the directory containing clang_rt.profile-x86_64.lib. For example my mozconfig has:
export LIB="$LIB;D:\\src\\llvm8\\lib\\clang\\8.0.0\\lib\\windows"

Comment 26

[koboltzz]

That's it, thanks for fast answer and help. I don't know if you hear this often, but despite the criticism that sometimes comes up (giving up XUL, the DNS cloudflare thingy etc.), you're all doing a great job. I enjoy using firefox every day.

Comment 27

[koboltzz]

okay, praised too early ;) 
Almost everything seemed to be compiling smoothly, the automatic tests were running successfully and then 15 minutes later I get:

lld-link.exe: error: could not open c:/mozilla-source/obj/cygprofile.txt: no such file or directory
mozmake.EXE[5]: *** [c:/mozilla-source/mozilla-inbound/config/rules.mk:697: xul.dll] Error 1
mozmake.EXE[4]: *** [c:/mozilla-source/mozilla-inbound/config/recurse.mk:74: toolkit/library/target] Error 2
mozmake.EXE[4]: *** Waiting for unfinished jobs....
warning: no profile data available for file "SystemInfo.cpp" [-Wprofile-instr-unprofiled]
1 warning generated.
warning: no profile data available for file "validationES31.cpp" [-Wprofile-instr-unprofiled]
1 warning generated.
mozmake.EXE[3]: *** [c:/mozilla-source/mozilla-inbound/config/recurse.mk:34: compile] Error 2
mozmake.EXE[2]: *** [c:/mozilla-source/mozilla-inbound/config/rules.mk:423: default] Error 2
mozmake.EXE[1]: *** [Makefile:235: profiledbuild] Error 2
mozmake.EXE: *** [client.mk:150: build] Error 2
431 compiler warnings present.

BTW no way to edit comments here?

Comment 28

[(Away)]

That's really more of a fallout from bug 1444171 rather than this one. At this point we're starting to stray from the purpose of this bug ticket; I encourage you to ask in #build on Mozilla's IRC if you're still having trouble.