Page MenuHomePhabricator

CMake changes to get Windows self-host with PGO working
Needs ReviewPublic

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

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

Do you want to modify this as well?

843

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

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?