Support PGO in clang-cl builds

RESOLVED FIXED in Firefox 63

Status

defect
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: dmajor, Assigned: dmajor)

Tracking

(Depends on 1 bug)

unspecified
mozilla63
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

2 years ago
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.
Assignee

Updated

2 years ago
Attachment #8839799 - Attachment is patch: false

Comment 2

2 years ago
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>...
Assignee

Comment 3

2 years ago
What other linker would you use? The documentation is not especially clear about this.
Presumably, clang-cl pgo doesn't work at link time like MSVC's (assuming it works like clang pgo)

Comment 5

2 years ago
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...
Assignee

Comment 6

2 years ago
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!
Assignee

Comment 7

2 years ago
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
Assignee

Updated

2 years ago
No longer blocks: linker-lld
Summary: Look into doing clang-cl PGO builds with lld-link → Support PGO in clang-cl builds
Assignee

Comment 8

2 years ago
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
Assignee

Updated

2 years ago
Depends on: winclang
Assignee

Comment 9

Last year
> 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.

Updated

Last year
Product: Core → Firefox Build System
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
Assignee

Updated

Last year
See Also: → 1475074
Assignee

Comment 11

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

Updated

11 months ago
Depends on: 1478903
Assignee

Updated

11 months ago
Depends on: 1479800
Assignee

Updated

11 months ago
Depends on: 1479842
Assignee

Comment 12

11 months ago
32-bit is blocked by bug 1479800 that I don't want to wait for.
Assignee: nobody → dmajor
Attachment #8996716 - Flags: review?(core-build-config-reviews)
Attachment #8996716 - Flags: review?(core-build-config-reviews) → review+

Comment 13

11 months ago
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2011dae40293
Enable PGO in 64-bit clang-cl builds. r=froydnj

Comment 15

11 months ago
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a3882f27aa3
Enable PGO in 64-bit clang-cl builds. r=froydnj

Comment 17

11 months ago
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/084b1e903489
Enable PGO in 64-bit clang-cl builds. r=froydnj
Assignee

Updated

11 months ago
Flags: needinfo?(dmajor)

Comment 18

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/084b1e903489
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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 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.
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
Assignee

Updated

11 months ago
Blocks: 1443590
Assignee

Comment 22

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

Comment 23

11 months ago
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

11 months ago
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?
Assignee

Updated

11 months ago
Depends on: 1483524
Assignee

Comment 25

11 months ago
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

11 months ago
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

11 months ago
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?
Assignee

Comment 28

11 months ago
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.
Assignee

Updated

11 months ago
Blocks: 1483779
You need to log in before you can comment on or make changes to this bug.