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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

- I think MultiSource/xxx is the wrong choice here. Things in that directory all produce some stdout/stderr output that is then compared against the reference_output file.
- Todays lit time measurements are performed "from the outside" i.e. we time how long the process takes. My understanding of google benchmarks is that it designed as a microbenchmarking library that runs a small benchmark a varying number of times until certain confidence levels are reached. This means the process as a whole will have different runtimes depending on how fast confidence levels are reached and the numbers measured are probably not helpful because of that. (From skimming through google test documentation, the number of iterations varies and the number of repetitions (`--benchmark_repetitions=10`) is one level higher, yes?

Don't get me wrong: I welcome the addition of a microbenchmarking library to the llvm test-suite and I can already think of a few other uses/benchmarks that shouuld be converted to it. But we should get it right:

- IMO this should go into a custom subdirectory as it is a fundamentally different way to run benchmarks. Maybe `/Micro`.
- This should have a new strategy for running the benchmarks and extracting the resulting numbers. So it should have an extension in `test-suite/litsupport`.
- As long as this isn't worked out we could also keep it in a separate repository: The test-suite is modular nowadays: The test-suite cmake searches for subdirectories containining CMakeLists.txt and includes those automatically. So using an extra repository is as easy as checking it out into the test-suite directory to have it picked up.



================
Comment at: MultiSource/Benchmarks/XRay/CMakeLists.txt:6-7
+
+set(PROG retref-bench-O0)
+list(APPEND CPPFLAGS -O0)
+llvm_multisource()
----------------
dberris wrote:
> kristof.beyls wrote:
> > It looks weird to me that this CMakeFile explicitly set different targets for -O0, -O1, -O2, -O3. I don't think other programs in the test-suite do this.
> > Do you have a good reason to do it like this?
> > It seems to go slightly against the general philosophy of making the test-suite an as-normal-as-possible cmake project; where you would set optimization level during cmake configuration; and if you want to collect data for multiple optimization levels build the test-suite multiple times with different cmake configurations.
> > @MatzeB : do you have thoughts on this?
> Now that you mention it, this does seem weird. This is mostly a faithful reproduction of my attempts at doing this with a shell script. @eizan has graciously volunteered to port this into something that can be part of the LLVM test suite.
> 
> If it would be more acceptable to just have a single target, then that's something we ought to change this to.
I agree, we rather run the benchmark suite multiple times instead of hardcoding optimization flags. (So we can also test -Os, -Oz, -Og, -mllvm -enable-my-cool-optimization or whatever someone cares about)


================
Comment at: libs/CMakeLists.txt:2-8
+ExternalProject_Add(
+                  google_benchmark
+                  SOURCE_DIR ${CMAKE_SOURCE_DIR}/libs/benchmark-1.1.0
+                  CMAKE_COMMAND cmake
+                  CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=${CMAKE_BINARY_DIR}/benchmark -DCMAKE_BUILD_TYPE=Release
+                  INSTALL_DIR ${CMAKE_BINARY_DIR}/benchmark
+)
----------------
Using ExternalProject is always a bit problematic as it is not clear how flags get propagated from the top to the subprojects. Libraries are not part of the target/namespace (see above where you use the full path to libbenchmark.a) etc.

This should rather be rewritten to build the google benchmark library as a part of the main test-suite build.


================
Comment at: lit.cfg:15
 config.single_source = False
+config.environment['XRAY_OPTIONS'] = 'patch_premain=false xray_naive_log=false'
 if 'SSH_AUTH_SOCK' in os.environ:
----------------
This is a bad fit here as it will be passed to all benchmarks and on the other hand hard to discover when you just look at the xray benchmarks files. You should rather pass this specifically to the xray benchmarks only.


https://reviews.llvm.org/D32272