Closed Bug 1341525 Opened 7 years ago Closed 6 years ago

Support PGO in clang-cl builds

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: away, Assigned: away)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

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.
Attachment #8839799 - Attachment is patch: false
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>...
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)
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...
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!
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
No longer blocks: linker-lld
Summary: Look into doing clang-cl PGO builds with lld-link → Support PGO in clang-cl builds
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
Depends on: winclang
> 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.
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
See Also: → 1475074
I finally managed to run this through creduce. Filed as https://bugs.llvm.org/show_bug.cgi?id=38251
Depends on: 1478903
Depends on: 1479800
Depends on: 1479842
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+
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2011dae40293
Enable PGO in 64-bit clang-cl builds. r=froydnj
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a3882f27aa3
Enable PGO in 64-bit clang-cl builds. r=froydnj
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/084b1e903489
Enable PGO in 64-bit clang-cl builds. r=froydnj
Flags: needinfo?(dmajor)
https://hg.mozilla.org/mozilla-central/rev/084b1e903489
Status: NEW → RESOLVED
Closed: 6 years 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
Depends on: 1480708
Blocks: 1443590
In addition to making this faster this also made the installer 2.8MB/4% smaller.
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
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?
Depends on: 1483524
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"
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.
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?
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.
Blocks: 1483779
You need to log in before you can comment on or make changes to this bug.