Differential D62063
CMake changes to get Windows self-host with PGO working Authored by russell.gallop on May 17 2019, 6:40 AM.
Details This:
Say if this should be split up further.
Diff Detail Event TimelineComment Actions 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. Comment Actions 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?
Comment Actions 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.
Comment Actions 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. But I believe that the cmake changes are okay. Comment Actions
I think that this has already been reported as https://bugs.llvm.org/show_bug.cgi?id=41279. Comment Actions
Created here: https://reviews.llvm.org/D62200 Comment Actions 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? Comment Actions 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? Comment Actions 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? Comment Actions 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-llvmMaybe 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. Comment Actions
Actually, I'll just do that for now. r372226. Comment Actions 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 | |||||||||||||||
Do you want to modify this as well?