Page MenuHomePhabricator

Pull google/benchmark library to the LLVM tree
ClosedPublic

Authored by kbobyrev on Aug 17 2018, 6:26 AM.

Details

Summary

This patch pulls google/benchmark v1.4.1 into the LLVM tree so that any project could use it for benchmark generation. A dummy benchmark is added to llvm/benchmarks/DummyYAML.cpp to validate the correctness of the build process.

The current version does not utilize LLVM LNT and LLVM CMake infrastructure, but that might be sufficient for most users. Two introduced CMake variables:

  • LLVM_INCLUDE_BENCHMARKS (ON by default) generates benchmark targets
  • LLVM_BUILD_BENCHMARKS (OFF by default) adds generated benchmark targets to the list of default LLVM targets (i.e. if ON benchmarks will be built upon standard build invocation, e.g. ninja or make with no specific targets)

List of modifications:

  • BENCHMARK_ENABLE_TESTING is disabled
  • BENCHMARK_ENABLE_EXCEPTIONS is disabled
  • BENCHMARK_ENABLE_INSTALL is disabled
  • BENCHMARK_ENABLE_GTEST_TESTS is disabled
  • BENCHMARK_DOWNLOAD_DEPENDENCIES is disabled

Original discussion can be found here: http://lists.llvm.org/pipermail/llvm-dev/2018-August/125023.html

Diff Detail

Repository
rL LLVM

Event Timeline

kbobyrev created this revision.Aug 17 2018, 6:26 AM
kbobyrev edited the summary of this revision. (Show Details)Aug 18 2018, 4:45 AM
dberris accepted this revision.Aug 20 2018, 7:02 PM

LGTM -- probably want to wait for someone else to review further, but I do have one question.

llvm/CMakeLists.txt
501 ↗(On Diff #161226)

Is there a way to run these automatically as well, similar to the split between the LLVM_BUILD_TESTS and LLVM_INCLUDE_TESTS?

This revision is now accepted and ready to land.Aug 20 2018, 7:02 PM
kbobyrev edited reviewers, added: chandlerc; removed: ioeric, ilya-biryukov.Aug 21 2018, 4:57 AM
kbobyrev edited subscribers, added: ioeric, ilya-biryukov; removed: chandlerc.
lebedev.ri added inline comments.Aug 21 2018, 5:04 AM
llvm/utils/benchmark/BUILD.bazel
2 ↗(On Diff #161226)

Based on previous expirience (rL336635/rL336666), i think you want to drop the bazel stuff.

llvm/utils/benchmark/tools/compare_bench.py
3 ↗(On Diff #161226)

Consider not adding this script/file in the first place, only the benchmark/tools/compare.py, which superseded/replaced this one.
(benchmark/tools/compare_bench.py is already removed in gbench git master)

kbobyrev added inline comments.Aug 21 2018, 6:21 AM
llvm/CMakeLists.txt
501 ↗(On Diff #161226)

Do I understand correctly that LLVM_INCLUDE_TESTS adds tests to the list of default targets while LLVM_BUILD_TESTS simply enables/disables them?

Would you suggest the same split for benchmarks? I thought that for the first iteration we might want to leave the benchmarks "invisible" for the most users, but I can totally see how excluding them from the global list of build targets (which is the case now) can be hard for people to actually start running benchmarks.

kbobyrev updated this revision to Diff 161708.EditedAug 21 2018, 6:22 AM

Delete BAZEL build files, remove deprecated compare_bench.py.

lebedev.ri added inline comments.Aug 21 2018, 6:24 AM
llvm/utils/benchmark/test/BUILD
1 ↗(On Diff #161708)

This one too.

kbobyrev updated this revision to Diff 161709.Aug 21 2018, 6:25 AM
kbobyrev marked an inline comment as done.
dberris added inline comments.Aug 22 2018, 6:55 AM
llvm/CMakeLists.txt
501 ↗(On Diff #161226)

Yes, I'd suggest having the split in there. It's one thing to build the benchmarks (as we have build bots that do a "fast" build) and another to run them. I don't yet see whether we will have performance runners collecting data as with the test-suite nightly runners, but even if we don't have those, at least having an option for just building the benchmarks being different from running them will be useful.

From NetBSD point of view, this looks fine, 1.4.1 contains upstreamed support.

kbobyrev updated this revision to Diff 162331.Aug 24 2018, 12:55 AM
kbobyrev marked 3 inline comments as done.

Similarly to unittest infrastructure, add LLVM_BUILD_BENCHMARKS and LLVM_INCLUDE_BENCHMARKS CMake variables to add benchmarks to the list of default targets and generate build targets respectively.

Next step (for another patch) would be to add update benchmark library according to https://github.com/google/benchmark/commit/f85304e4e3a0e4e1bf15b91720df4a19e90b589f to silence a bunch of warnings generated by a relatively recent Clang compiler.

lebedev.ri added inline comments.Aug 24 2018, 1:00 AM
llvm/CMakeLists.txt
496–497 ↗(On Diff #162331)

This looks weird. Is LLVM_INCLUDE_BENCHMARKS=on==LLVM_BUILD_BENCHMARKS=off?
Both of these talk about build targets. I suspect what you meant is

option(LLVM_BUILD_BENCHMARKS "Build LLVM benchmarks. If OFF, just generate build targets" OFF)
option(LLVM_INCLUDE_BENCHMARKS "Execute LLVM benchmarks as part of tests. If OFF, just generate benchmarking targets" OFF)
kbobyrev updated this revision to Diff 162332.Aug 24 2018, 1:07 AM
kbobyrev marked an inline comment as done.

@lebedev.ri you're right, the wording is not optimal.

I also added documentation for the introduced CMAKE variables.

kbobyrev updated this revision to Diff 162333.Aug 24 2018, 1:12 AM

Introduce Benchmarks target and add every new benchmark to this target.

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.

llvm/CMakeLists.txt
1020 ↗(On Diff #162332)

But this should check LLVM_BUILD_BENCHMARKS?

llvm/docs/CMake.rst
253–254 ↗(On Diff #162332)

Bread: similar to bread, consists of bread :)

253–258 ↗(On Diff #162332)

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.

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).

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?
Is LLVM_BUILD_BENCHMARKS supposed to control whether the targets that are added by LLVM_INCLUDE_BENCHMARKS are part of all build target?
If yes, then should LLVM_BUILD_BENCHMARKS=on imply LLVM_INCLUDE_BENCHMARKS=on ?
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)

kbobyrev updated this revision to Diff 162343.Aug 24 2018, 2:01 AM
kbobyrev marked an inline comment as done.
kbobyrev edited the summary of this revision. (Show Details)

Do a better job at describing what the CMake variables actually do.

kbobyrev added a comment.EditedAug 24 2018, 2:06 AM

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 the same with tests.

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.

llvm/CMakeLists.txt
1020 ↗(On Diff #162332)

No, this is the same as LLVM_INCLUDE_TESTS, it does the same thing.

llvm/docs/CMake.rst
253–258 ↗(On Diff #162332)

Neither. It controls adding build targets, that's what the description says, too.

Ok great, that makes more sense :)

llvm/CMakeLists.txt
496 ↗(On Diff #162343)

Does stringsplit actually works that way in cmake? I usually did "sti" newline "ng".

llvm/cmake/modules/AddLLVM.cmake
1129 ↗(On Diff #162343)

Are you sure this is needed?
target_link_libraries(${benchmark_name} PRIVATE benchmark) should have already be sufficient. (if not, there is a bug)

1135 ↗(On Diff #162343)

Uppercase target name is unusual. Is there a precedent/reasoning for this?

kbobyrev updated this revision to Diff 162349.Aug 24 2018, 2:16 AM
kbobyrev marked 6 inline comments as done.

Address few nits.

llvm/cmake/modules/AddLLVM.cmake
1135 ↗(On Diff #162343)

Yes, there's *UnitTests* target.

kbobyrev updated this revision to Diff 162353.Aug 24 2018, 2:36 AM
kbobyrev marked 2 inline comments as done.

Get rid of Benchmarks target, which doesn't work for other projects.

Should an llvm/utils/benchmark/README.LLVM be added, like llvm/utils/unittest/googletest/README.LLVM ?

llvm/utils/benchmark/CMakeLists.txt
15 ↗(On Diff #162353)

There is a very high chance these changes will be lost during drop-in updates.
It may be best to set() them in llvm/CMakeLists.txt

Should an llvm/utils/benchmark/README.LLVM be added, like llvm/utils/unittest/googletest/README.LLVM ?

Oh, but it already is!

lebedev.ri accepted this revision.Aug 24 2018, 6:14 AM

LG other than those last nits.

llvm/CMakeLists.txt
496 ↗(On Diff #162343)

(It was marked as done without a change, or a comment.)

llvm/utils/benchmark/README.LLVM
4–5 ↗(On Diff #162353)

Ah nice. Somehow i have seen it before, but then forgot.
slight CMake script modifications part worries me.
Either those changes need to be documented here, or (best) there shouldn't be any changes,
since you can toggle them via set() natively.

kbobyrev updated this revision to Diff 162368.Aug 24 2018, 6:20 AM
kbobyrev edited the summary of this revision. (Show Details)

Forward necessary option values to benchmark instead of manually modifying CMake script in the benchmark source directory.

kbobyrev updated this revision to Diff 162369.Aug 24 2018, 6:28 AM
kbobyrev marked 2 inline comments as done.
  • Update README.LLVM: it should only mention that BUILD files are removed, CMake script at benchmark's root is no longer changed after arguments forwarding
  • Don't use \ for line continuation in LLVM's CMake root.
llvm/CMakeLists.txt
496 ↗(On Diff #162343)

Ah, sorry, there was a misunderstanding then. I thought what you meant is that the continuation with \ doesn't append space and I should manually do that.

I think, both work: https://cmake.org/cmake/help/v3.0/manual/cmake-language.7.html#quoted-argument

Your variant seems to be more straightforward, though.

Since there are no objections or concerns raised, I will submit the patch.

This revision was automatically updated to reflect the committed changes.
a.elovikov added inline comments.
llvm/trunk/utils/benchmark/src/CMakeLists.txt
14
lebedev.ri added inline comments.Aug 28 2018, 3:50 AM
llvm/trunk/utils/benchmark/src/CMakeLists.txt
14