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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.
This revision is now accepted and ready to land.

See comments below but nothing blocking. LGTM.

In https://reviews.llvm.org/D32272#795123, @eizan wrote:

> - Move libs/benchmark-1.0.0 into MicroBenchmarks
> - Guard building of all microbenchmarks based on ...ENABLE_XRAY_MICROBENCHMARKS
> - Rename ...ENABLE_XRAY_MICROBENCHMARKS -> ...ENABLE_MICROBENCHMARKS


I don't think we need TEST_SUITE_ENABLE_MICROBENCHMARKS there is already TEST_SUITE_SUBDIRS. Just change the toplevel CMakeLists.txt to read like this:

  ...
     # Exclude tools, CTMark and MicroBenchmarks from default list
      if(NOT ${subdir} STREQUAL tools AND NOT ${subdir} STREQUAL CTMark
         AND NOT ${subdir} STREQUAL MicroBenchmarks)
        list(APPEND TEST_SUITE_SUBDIRS ${subdir})
      endif()
  ...

You would then use -DTEST_SUITE_SUBDIRS="MicroBenchmarks" or -DTEST_SUITE_SUBDIRS="SingleSource;MultiSource;MicroBenchmarks" to enable it so we don't need a separate TEST_SUITE_ENABLE_MICROBENCHMARKS flag.

> A few notes/questions that I'd like to bring to attention:
> 
> - I've interpreted the suggestion to be very careful to not build benchmark-1.1.0 unless the XRay benchmarks that depend on it are built. Unless I guard the add_directory(libs) with a conditional in MicroBenchmarks/CMakeLists.txt, when running the full benchmark suite without ENABLE_MICROBENCHMARKS=1, the benchmark library gets built even if the XRay binaries aren't built. Is there a better way?

It is fine as is. The library won't be built if the MicroBenchmarks are skipped as a whole.

> - I had to hack in add_dependencies(benchmark, timeit-host), otherwise the benchmkark build would fail due to it being built before timeit. Is there a better way?

You can use `test_suite_add_build_dependencies(TARGETNAME)` to get some abstraction.

> - I'm somewhat flippantly presuming that all the targets in MicroBenchmarks will depend on the benchmark-1.1.0 lib. Is this okay?

Seems reasonable to me.


https://reviews.llvm.org/D32272