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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

- We should really add a `litsupport/` plugin rather than starting an LNTBased suite. I see no advantage of using LNTBased, but at the same time writing a new runner from scratch means you have to reimplement (or not implement) codesize, compiletime measurements, pgo data collection feeding back into the build etc.



================
Comment at: CMakeLists.txt:155
 add_subdirectory(tools)
+add_subdirectory(MicroBenchmarks)
 # Shortcut for the path to the fpcmp executable
----------------
Why do you unconditionally include `MicroBenchmarks` instead of using the existing TEST_SUITE_SUBDIRS mechanism?


================
Comment at: MicroBenchmarks/CMakeLists.txt:1-4
+check_cxx_compiler_flag(-fxray-instrument COMPILER_HAS_FXRAY_INSTRUMENT)
+if("${ARCH}" STREQUAL "x86" AND ${COMPILER_HAS_FXRAY_INSTRUMENT})
+  add_subdirectory(XRay)
+endif()
----------------
It seems like the x86 and xray checks should rather go into the xray directory. I can easily imagine other microbenchmarks getting added that do not depend on those flags.


================
Comment at: MicroBenchmarks/XRay/CMakeLists.txt:8-9
+llvm_multisource()
+#add_dependencies(retref-bench benchmark)
+#include_directories ("${CMAKE_BINARY_DIR}/benchmark/include")
+target_link_libraries(retref-bench benchmark)
----------------
Please don't add commented code.


================
Comment at: MicroBenchmarks/XRay/lit.local.cfg:1
+config.environment['XRAY_OPTIONS'] = 'patch_premain=false xray_naive_log=false'
----------------
Don't you need some `file(COPY)` in CMakeLists.txt that copies this file into the builddir?


https://reviews.llvm.org/D32272