[PATCH] D62063: CMake changes to get Windows self-host with PGO working

Mon May 20 17:35:14 PDT 2019

smeenai added subscribers: beanz, smeenai.
smeenai added a comment.

In D62063#1509321 <https://reviews.llvm.org/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?

================
Comment at: llvm/cmake/modules/HandleLLVMOptions.cmake:834
   if (LLVM_ENABLE_IR_PGO OR uppercase_LLVM_BUILD_INSTRUMENTED STREQUAL "IR")
     append("-fprofile-generate='${LLVM_PROFILE_DATA_DIR}'"
       CMAKE_CXX_FLAGS
----------------
Do you want to modify this as well?

================
Comment at: llvm/cmake/modules/HandleLLVMOptions.cmake:840
   elseif(uppercase_LLVM_BUILD_INSTRUMENTED STREQUAL "CSIR")
     append("-fcs-profile-generate='${LLVM_CSPROFILE_DATA_DIR}'"
       CMAKE_CXX_FLAGS
----------------
And this.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62063/new/

https://reviews.llvm.org/D62063