[PATCH] D50894: Pull google/benchmark library to the LLVM tree

[Kirill Bobyrev via Phabricator via llvm-commits]

kbobyrev added a comment.

In https://reviews.llvm.org/D50894#1212104, @lebedev.ri wrote:

> In https://reviews.llvm.org/D50894#1212096, @kbobyrev wrote:
>
> > In https://reviews.llvm.org/D50894#1212052, @lebedev.ri wrote:
> >
> > > Those cmake options still look wrong to me.
> > >  It looks as-if you are trying to make these two options two three different things, with inconsistent mapping.
> >
> >
> > Could you please rephrase? I'm not sure I get the last sentence.
> >
> > Also, sorry, I was is completely wrong. I added your description but both descriptions weren't what the proposed CMake variables do.
> >
> > `LLVM_INCLUDE_BENCHMARKS` doesn't execute them after the build, just like `LLVM_INCLUDE_TESTS` doesn't execute tests, it simply determines whether build targets would be generated or not (i.e. whether `benchmarks` and `unittests` will be added via `add_subdirectory`).
>
>
> I'm confused because i do not understand what these two options are *meant* to do.
>  Is `LLVM_INCLUDE_BENCHMARKS` supposed to only control whether the //targets// to build the libbenchmark and the benchmark executables are generated or not?


Yes.

> Is `LLVM_BUILD_BENCHMARKS` supposed to control whether the targets that are added by `LLVM_INCLUDE_BENCHMARKS` are part of `all` build target?

Yes.

> If yes, then should `LLVM_BUILD_BENCHMARKS=on` imply `LLVM_INCLUDE_BENCHMARKS=on` ?

Yes, correct. But that's

> I'm confused because previously it was discussed that `LLVM_INCLUDE_BENCHMARKS` should control the **execution** of the built benchmarks themselves as part of the testing. (which i'm not sure will work)

Yes, I agree, it's a bit confusing. I think what @dberris proposed was execution of the benchmarks with the `LLVM_BUILD_BENCHMARKS`, but I'm not sure it's possible without lit integration (which is not there yet) and I also don't think it's a good idea because we don't have that for tests (tests execution is via `ninja/make check-all`, not just passing `LLVM_BUILD_TESTS`). Also, I would find "BUILD_BENCHMARKS" actually execute benchmarks very confusing.

I might have misunderstood what @dberris was suggesting, but I thought the plan was to do "the same" for benchmarks compared to the tests.



================
Comment at: llvm/CMakeLists.txt:1020
+
+if (LLVM_INCLUDE_BENCHMARKS)
+  add_subdirectory(utils/benchmark)
----------------
lebedev.ri wrote:
> But this should check `LLVM_BUILD_BENCHMARKS`?
No, this is the same as `LLVM_INCLUDE_TESTS`, it does the same thing.


================
Comment at: llvm/docs/CMake.rst:253-258
+**LLVM_BUILD_BENCHMARKS**:BOOL
+  Similar to ``LLVM_BUILD_BENCHMARKS``: adds benchmarks to the list of default
+  targets. Defaults to OFF.
+
+**LLVM_INCLUDE_BENCHMARKS**:BOOL
+  Generate build targets for the LLVM benchmarks. Defaults to ON.
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > Bread: similar to bread, consists of bread :)
> > 
> Also, there is a terminology issue. Does this control //building// of the benchmarks, or //execution// of benchmarks?
> I agree that it isn't clear from the `LLVM_INCLUDE_TESTS`/`LLVM_BUILD_TESTS` description, either.
Neither. It controls adding build targets, that's what the description says, too.


https://reviews.llvm.org/D50894