[PATCH] D15462: [CMake] Add support for generating profdata for clang from training files

[Chris Bieneman via cfe-commits]

beanz added a comment.

Sean,

The reason for restricting to Unix is two fold. (1) the shell script goop, which I can replace with python and (2) I don't have a windows box to test on, so I didn't want people to think it worked.

If I replace the shell goop with Python this will probably "Just Work" everywhere, so I can do that.

I also agree about your documentation point. That will need to be a separate patch.

More comments inline.


================
Comment at: utils/perf-training/CMakeLists.txt:2
@@ +1,3 @@
+if(LLVM_BUILD_INSTRUMENTED AND NOT WIN32)
+  if (CMAKE_CFG_INTDIR STREQUAL ".")
+    set(LLVM_BUILD_MODE ".")
----------------
vsk wrote:
> Afaict, lines 2-8 look like they're provided by configure_lit_site_cfg (AddLLVM.cmake).
configure_lit_site_cfg doesn't set CLANG_TOOLS_DIR, which needs to be set and uses this code. We should probably clean this all up so we don't need this duplication, but that is a separate problem.

================
Comment at: utils/perf-training/CMakeLists.txt:8
@@ +7,3 @@
+
+  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} CLANG_TOOLS_DIR ${LLVM_RUNTIME_OUTPUT_INTDIR})
+
----------------
vsk wrote:
> Is it possible for this line to interact poorly with the one in clang/test/CMakeLists.txt? E.g something weird like if CMAKE_CFG_INTDIR is a substring of LLVM_BUILD_MODE?
It won't interact with the other directory because CMake does level-based scoping. The tests are processed before this code, and the directory pops back out to the root before traversing this.

================
Comment at: utils/perf-training/CMakeLists.txt:17
@@ +16,3 @@
+    ${CMAKE_CURRENT_BINARY_DIR}
+    DEPENDS clang clear-profraw
+    )
----------------
vsk wrote:
> Can we build llvm-profdata too?
You actually don't want that. You want your llvm-profdata to match the profile runtime you're building with. Since this builds with the profile runtime coming with the compiler you're using (which is usually the system compiler) you'll want a system profdata tool.

I do plan to support bootstrap PGO generation where if you do a 3-stage build. In that world stage1 will be your toolchain to use to build the instrumented compiler and stage1 would provide llvm-profdata.

================
Comment at: utils/perf-training/CMakeLists.txt:21
@@ +20,3 @@
+  add_custom_target(clear-profraw
+    COMMAND find ${CMAKE_CURRENT_BINARY_DIR} -name "*.profraw" -print | xargs rm
+    COMMENT "Clearing old profraw data")
----------------
silvas wrote:
> Can you write a tiny pure-Python helper script for clear-profraw and another for generate-profdata? That will be more portable.
> 
> If this is the only thing blocking windows support, I think that shooting for windows support on the initial patch is worth a shot.
> 
> 
> 
I'll give it a go today.

================
Comment at: utils/perf-training/CMakeLists.txt:25
@@ +24,3 @@
+  if(NOT LLVM_PROFDATA)
+    find_program(LLVM_PROFDATA llvm-profdata)
+  endif()
----------------
vsk wrote:
> We should use a llvm-profdata built out of the current source. Does the `llvm_find_program` function help with this?
No, we really don't want to do that. See my comment above (Note: I had implemented that and it doesn't work).

================
Comment at: utils/perf-training/lit.cfg:7
@@ +6,3 @@
+
+def inferClang(PATH):
+    # Determine which clang to use.
----------------
vsk wrote:
> This duplicates code in tools/clang/test/lit.cfg. Can we lift it into a shared module?
This code probably isn't needed. Since it should always use just-built clang, I can probably just use lit.util.which directly instead of this wrapper. I'll make that change.

================
Comment at: utils/perf-training/lit.site.cfg.in:3
@@ +2,3 @@
+
+## Autogenerated by LLVM/Clang configuration.
+# Do not edit!
----------------
vsk wrote:
> I don't know too much about what's going on here. It seems weird to check in auto generated files.. why do we need to do that?
This file is actually the input to the auto-generation. It needs to be checked in. That comment is all over the lit.*.in files so that people know the outputted files are auto-generated.

This probably ins't super important anymore because we don't support in-source builds, but back in the day it was because you'd end up with all these auto-generated lit.site.cfg files all over you source directory.


http://reviews.llvm.org/D15462