Page MenuHomePhabricator

CMake changes to get Windows self-host with PGO working
ClosedPublic

Authored by russell.gallop on May 17 2019, 6:40 AM.

Details

Summary

This:

  • Fixes quoting of profile arguments to work on Windows
  • Suppresses adding profile arguments to linker flags when using lld-link
  • Avoids -fprofile-instr-use being added to rc.exe flags
  • Removes duplicated adding of -fprofile-instr-use to linker flags (since r355541)
  • Move handling LLVM_PROFDATA_FILE to HandleLLVMOptions.cmake

Say if this should be split up further.

Diff Detail

Repository
rL LLVM

Event Timeline

russell.gallop created this revision.May 17 2019, 6:40 AM
rnk added a comment.May 20 2019, 4:40 PM

Seems reasonable to me. I'm surprised CMake doesn't have a general facility for escaping paths used in cflags. I suppose users generally don't add paths, just flags.

In D62063#1509321, @rnk wrote:

Seems reasonable to me. I'm surprised CMake doesn't have a general facility for escaping paths used in cflags. I suppose users generally don't add paths, just flags.

CMAKE_*_FLAGS are just a straight-up string, so there's no automatic escaping going on there. The newer target options (e.g. target_compile_options) are lists, so I'd expect them to handle escaping properly.

It's way out of the scope of this diff, of course, but in general I believe setting CMAKE_*_FLAGS is considered to be an anti-pattern now, and using target-specific options is recommended. I'm not sure how to best do that for options which are supposed to apply to every single target though (like the instrumentation options). @beanz?

llvm/cmake/modules/HandleLLVMOptions.cmake
834 ↗(On Diff #200032)

Do you want to modify this as well?

840 ↗(On Diff #200032)

And this.

beanz added a comment.May 20 2019, 9:48 PM

The CMake goop all looks reasonable to me. I think the new "correct" way to alter the compiler flags is add_compile_options, but since LLVM pretty heavily relies on modifying CMAKE_<LANG>_FLAGS it is probably better to be consistent than to start doing something new here.

It might be worth someday migrating to add_compile_options, but frankly doing that is kinda messy because you need to use lots of generator expressions to get it right. For example an an equivalent call to add_compile_options for one of the append calls in this patch would be something like:

add_compile_options($<$<OR:$<COMPILE_LANGUAGE:CXX>,$<COMPILE_LANGUAGE:C>>:"-fprofile-instr-generate=\"${LLVM_PROFILE_FILE_PATTERN}\"">)

Someday we will need to do a big round of CMake modernization, but let's not start that here.

russell.gallop added inline comments.May 21 2019, 3:05 AM
llvm/cmake/modules/HandleLLVMOptions.cmake
834 ↗(On Diff #200032)

Ah! I thought that these options weren't supported by clang-cl. Looks like Hans has fixed that in r357255. I'll change these as well. Thanks.

Handle -fprofile-generate and -fcs-profile-generate options for self hosting as well.

This highlights two other problems with using those options for Windows self host builds.
1). -fprofile-generate and -fcs-profile-generate need clang_rt.profile adding with --dependent-lib in the driver (like https://reviews.llvm.org/D61742). I'll create a separate review for that.
2). Self hosting with -fprofile-generate hits a crash building CrashRecoveryContext.cpp. I'll try to report this.

But I believe that the cmake changes are okay.

russell.gallop marked 3 inline comments as done.May 21 2019, 7:00 AM

2). Self hosting with -fprofile-generate hits a crash building CrashRecoveryContext.cpp. I'll try to report this.

I think that this has already been reported as https://bugs.llvm.org/show_bug.cgi?id=41279.

1). -fprofile-generate and -fcs-profile-generate need clang_rt.profile adding with --dependent-lib in the driver (like https://reviews.llvm.org/D61742). I'll create a separate review for that.

Created here: https://reviews.llvm.org/D62200

https://reviews.llvm.org/D62200 has landed so -fprofile-generate and -fcs-profile-generate both add --dependent-lib (required for this patch to make sense).

This crash https://bugs.llvm.org/show_bug.cgi?id=41279 still prevents self hosting with IR instrumentation on Windows but I don't believe that should block this.

Okay to commit now?

Update: https://bugs.llvm.org/show_bug.cgi?id=41279 has been fixed by https://reviews.llvm.org/D62700.

Building self host with FE PGO works but IR PGO still doesn't work due to: https://bugs.llvm.org/show_bug.cgi?id=42370

Should we wait until all of the functionality works before this is submitted?

https://reviews.llvm.org/D67579 has fixed https://bugs.llvm.org/show_bug.cgi?id=42370 / https://bugs.llvm.org/show_bug.cgi?id=41380.

With this patch applied to r372087 I can now self host with FE and IR PGO on Windows.

Okay to commit now?

rnk edited reviewers, added: hans; removed: rnk.Sep 17 2019, 11:11 AM
rnk added a subscriber: rnk.

@hans has been trying to build clang with PGO on Windows, so I'll defer this to him.

hans accepted this revision.Sep 18 2019, 2:25 AM
In D62063#1673013, @rnk wrote:

@hans has been trying to build clang with PGO on Windows, so I'll defer this to him.

Looks great to me! Thanks for working on this.

This revision is now accepted and ready to land.Sep 18 2019, 2:25 AM
Closed by commit rL372209: [cmake] Changes to get Windows self-host working with PGO (authored by russell_gallop, committed by ). · Explain WhySep 18 2019, 2:41 AM
This revision was automatically updated to reflect the committed changes.
hans added a comment.Sep 18 2019, 6:46 AM

Sadly, this seems to break the lit tests on Mac when building with a profile.

Here's an attempt to do a bootstrap with PGO on Mac for Chromium: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8901987169223158016/+/steps/package_clang/0/stdout

It ends with:

[571/572] Running the LLVM regression tests
llvm-lit: /b/s/w/ir/cache/builder/src/third_party/llvm/llvm/utils/lit/lit/TestingConfig.py:102: fatal: unable to parse config file '/b/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/test/lit.site.cfg.py', traceback: Traceback (most recent call last):
  File "/b/s/w/ir/cache/builder/src/third_party/llvm/llvm/utils/lit/lit/TestingConfig.py", line 89, in load_from_path
    exec(compile(data, path, 'exec'), cfg_globals, None)
  File "/b/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/test/lit.site.cfg.py", line 33
    config.host_ldflags = "-stdlib=libc++ -fno-pie -fprofile-instr-use="/b/s/w/ir/cache/builder/src/third_party/llvm-instrumented/profdata.prof""
                                                                                                                                                ^
SyntaxError: invalid syntax

FAILED: test/CMakeFiles/check-llvm

Maybe config.host_ldflags should be using single quotes? Or actually maybe that flag shouldn't end up in host_ldflags at all, though I'm not sure how one would fix that.

hans added a comment.Sep 18 2019, 7:11 AM

Maybe config.host_ldflags should be using single quotes?

Actually, I'll just do that for now. r372226.

hans added a comment.Sep 19 2019, 2:32 AM

Sadly, we're seeing more breakages on Mac, where CMake test compiles for e.g. armv7 ios are failing with:

Testing compiler for supporting ios-armv7:
...
^[[0;1;31merror: ^[[0mCould not read profile "/tmp/foo.profdata": No such file or directory^[[0m
1 error generated.

(Note the extra quotes that are breaking the filename.)

I'm trying to debug this over at https://bugs.chromium.org/p/chromium/issues/detail?id=1005628

hans added a comment.Sep 19 2019, 4:26 AM

I've committed a horrible workaround in r372312.