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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
