Support PGO in clang-cl builds

RESOLVED FIXED in Firefox 63

Status

RESOLVED FIXED
2 years ago
6 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)

Comment 1

2 years ago
Created attachment 8839799 [details]
Toy program showing build steps
(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: 1384434
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

a year ago
Depends on: 752004
(Assignee)

Comment 9

a year ago
> 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

a year ago
Product: Core → Firefox Build System

Comment 10

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

8 months ago
See Also: → bug 1475074
(Assignee)

Comment 11

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

Updated

7 months ago
Depends on: 1478903
(Assignee)

Updated

7 months ago
Depends on: 1479800
(Assignee)

Updated

7 months ago
Depends on: 1479842
(Assignee)

Comment 12

7 months ago
Created attachment 8996716 [details] [diff] [review]
Enable PGO in 64-bit clang-cl builds

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

7 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

7 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

7 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

7 months ago
Flags: needinfo?(dmajor)

Comment 18

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/084b1e903489
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox63: --- → fixed
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

7 months ago
Blocks: 1443590
(Assignee)

Comment 22

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

Comment 23

7 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

6 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

6 months ago
Depends on: 1483524
(Assignee)

Comment 25

6 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

6 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

6 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

6 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

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