[PATCH] D32272: [XRay] Add Google Benchmark library + initial XRay benchmarks

Dean Michael Berris via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Apr 26 20:21:41 PDT 2017


dberris requested changes to this revision.
dberris added a comment.
This revision now requires changes to proceed.

Sorry for the delay @eizan -- some thoughts on the CMake config.



================
Comment at: MultiSource/Benchmarks/CMakeLists.txt:22
 add_subdirectory(DOE-ProxyApps-C)
+add_subdirectory(XRay)
 
----------------
We really only ought to include XRay if we can actually run the benchmarks. I'd say we can only do this for `x86_64` and only if we can actually build the XRay benchmarks with XRay instrumentation.


================
Comment at: MultiSource/Benchmarks/XRay/CMakeLists.txt:1
+list(APPEND CPPFLAGS -std=c++11 -Wl,--gc-sections -fxray-instrument)
+set(Source retref-bench.cc)
----------------
I think we need to detect first whether the compiler supports the `-fxray-instrument` flag in some sort of config or check. Something similar to the `cmake/config-ix.cmake` stuff in compiler-rt that detects whether the compiler can support the `-fxray-instrument` flag.


================
Comment at: MultiSource/Benchmarks/XRay/CMakeLists.txt:11
+include_directories ("${CMAKE_BINARY_DIR}/benchmark/include")
+target_link_libraries(retref-bench-O0 ${CMAKE_BINARY_DIR}/benchmark/lib/libbenchmark.a pthread)
+
----------------
Is there a way to add a dependency to the benchmark library instead, through some indirection -- maybe the benchmark lib exposes a variable that defines where the static library is?

Also, you probably want to add the following instead of `pthread` directly: `${CMAKE_THREAD_LIBS_INIT}`


https://reviews.llvm.org/D32272





More information about the llvm-commits mailing list