This change:
- moves the libcxx copy of google/benchmark to third-party/benchmkark
- points the 2 uses of the library (libcxx and llvm/utils) to this copy
We picked the licxx copy because it is the most up to date.
Paths
| Differential D112012
[benchmarks] Move libcxx's fork of google/benchmark and llvm/utils' under third-party ClosedPublic Authored by mtrofin on Oct 18 2021, 9:12 AM.
Details
Summary This change:
We picked the licxx copy because it is the most up to date.
Diff Detail
Event TimelineHerald added subscribers: jdoerfert, s.egerton, mstorsjo and 7 others. · View Herald TranscriptOct 18 2021, 9:12 AM Comment Actions I like this a lot in spirit, thanks! However, we are trying to decouple the runtimes (which include libc++) from everything in <monorepo>/llvm, and placing the GoogleBenchmark library inside <monorepo>/llvm somewhat goes against that. Would it make sense to try to find a place in LLVM where we can put third-party libraries like that? IMO it would be sensible to avoid tying them to <monorepo>/llvm just like we should avoid tying them to any other specific sub-project under the LLVM Foundation. WDYT? This revision now requires changes to proceed.Oct 18 2021, 1:08 PM Comment Actions @ldionne What is your concrete proposal? Requiring Google Benchmark pre-installed on the host? I think <root>/llvm is different in that it is not a subproject but contains core and supporting infrastructure (such as <root>/llvm/cmake/Modules, llvm-lit etc). Google Benchmark would be one of that. I don't think we should reject patches that just follow the status quo without having a plan what the future direction should be. Comment Actions
@ldionne @Meinersbur - trying to move this forward - if libcxx can't depend on llvm/ stuff, how about having a llvm/third-party common dir, would that work? (I'd want to circle this back to the thread discussing google-benchmark consolidation, but wanted to get a quick smoke test.) Comment Actions
At least it would also need changes in buidbot/zorg -- currently, changes in directories not related to any subproject will not trigger a new build. Comment Actions moved to third_party since the change is large because of the moved files, the important changes are in: llvm/CMakeLists.txt Comment Actions
As I said in my comment, I was suggesting that we put it in the monorepo, but just not under llvm/. You current proposal for third-party sounds reasonable to me.
This is historical, back when we would check out all the sub projects under llvm/projects. It makes no sense at all to store the common CMake infrastructure or lit under llvm/ anymore, either. I'm not suggesting we change the whole world right now, I'm just saying that if we are going to move stuff around, we might as well do it correctly from the start.
From the libc++ perspective, the patch before moving to third-party/ was adding a new hard dependency from libcxx/ to llvm/, which is precisely what we're trying to move away from, hence my question and my request for changes. IMO this patch looks great now, if everybody is happy with having a top-level third-party. I think it makes a lot of sense to have that, but I'm looking for other folks on this review to chime in. If we want to establish this as the new "canonical" place to store third-party dependencies, perhaps a post on the various lists would be a good idea to get people's attention? I'm not requesting it, just suggesting. Personally I'm happy with this change as-is. However, we do need to fix the CI for libc++. Since you're moving the library to another place, we'll need to adjust the paths to it in libcxx/. I think this should be sufficient: diff --git a/libcxx/benchmarks/CMakeLists.txt b/libcxx/benchmarks/CMakeLists.txt index cad880858a78..91aadc864907 100644 --- a/libcxx/benchmarks/CMakeLists.txt +++ b/libcxx/benchmarks/CMakeLists.txt @@ -35,7 +35,7 @@ ExternalProject_Add(google-benchmark-libcxx EXCLUDE_FROM_ALL ON DEPENDS cxx cxx-headers PREFIX benchmark-libcxx - SOURCE_DIR ${LIBCXX_SOURCE_DIR}/utils/google-benchmark + SOURCE_DIR ${LIBCXX_SOURCE_DIR}/../third-party/google-benchmark INSTALL_DIR ${CMAKE_CURRENT_BINARY_DIR}/benchmark-libcxx CMAKE_CACHE_ARGS -DCMAKE_C_COMPILER:STRING=${CMAKE_C_COMPILER} @@ -60,7 +60,7 @@ if (LIBCXX_BENCHMARK_NATIVE_STDLIB) ExternalProject_Add(google-benchmark-native EXCLUDE_FROM_ALL ON PREFIX benchmark-native - SOURCE_DIR ${LIBCXX_SOURCE_DIR}/utils/google-benchmark + SOURCE_DIR ${LIBCXX_SOURCE_DIR}/../third-party/google-benchmark INSTALL_DIR ${CMAKE_CURRENT_BINARY_DIR}/benchmark-native CMAKE_CACHE_ARGS -DCMAKE_C_COMPILER:STRING=${CMAKE_C_COMPILER} I suspect you need similar changes elsewhere in LLVM. Marking as approved in case I don't see this again for a while, but please don't submit until (at least) the libc++ CI is green. This revision is now accepted and ready to land.Nov 3 2021, 9:54 AM Comment Actions
Oh, sorry, I hadn't seen your changes to libcxx/benchmark/CMakeLists.txt. The issue now is that LLVM_THIRD_PARTY_DIR is only set in llvm/CMakeLists.txt. However, the runtimes don't use that directory anymore (in fact building the runtimes using llvm/CMakeLists.txt is deprecated). You'd have to set LLVM_THIRD_PARTY_DIR in runtimes/CMakeLists.txt. Comment Actions
Ah, I see - so in runtimes/CMakeLists.txt, if LLVM_THIRD_PARTY_DIR isn't set, then we set it there to ../third_party or something of that sort, correct? This revision now requires review to proceed.Nov 8 2021, 8:19 AM mtrofin retitled this revision from [benchmarks] Unify libcxx's fork of google/benchmark to llvm/utils to [benchmarks] Move libcxx's fork of google/benchmark and llvm/utils' under third-party.Nov 9 2021, 8:14 AM Comment Actions To fix the CI, you'll want to rebase onto main once I've landed D113503. That patch removes support for the benchmarks under the libc++ standalone build, since this patch breaks that use case. I think it's fine though, we are trying to get rid of the standalone build anyway, it's just a matter of time.
This revision now requires changes to proceed.Nov 9 2021, 11:47 AM mtrofin marked 3 inline comments as done. Comment Actionsno need to make third-party a project
mtrofin added inline comments. Comment Actions LGTM. Now that seems like a minimal change! Thanks! You can ignore the libc++ CI which failed, it failed due to an issue that has already been resolved. This revision is now accepted and ready to land.Nov 16 2021, 8:50 AM This revision was landed with ongoing or failed builds.Nov 16 2021, 9:16 AM Closed by commit rGc6f7b720ecfa: [benchmarks] Move libcxx's fork of google/benchmark and llvm/utils' (authored by mtrofin). · Explain Why This revision was automatically updated to reflect the committed changes. Comment Actions FYI: The libcxx copy and llvm/utils copy were not identical. So, the libc benchmarks are currently broken because of that. I have added an inline comment to show the single difference we are seeing. We can fix forward.
mtrofin added subscribers: wanders, • Quuxplusone, bhuvanendrakumarn and 28 others.Nov 16 2021, 3:36 PM Comment ActionsOh! Huh, missed that. Probably my ninja check-all didn’t build the Comment Actions This change broke Fuchsia's linux and mac clang builder. Failed build can be seen at: https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8830380874445931681/overview Comment Actions
I have already fixed the libc build. Comment Actions
Oh - thanks. (For some reason I'm getting messages out of order) should we revert though your fix until I reland this - the revert due to Fuchsia's build failure seems to be due to the way they ingest CMakeLists.txt Comment Actions
@ldionne @phosek I think the Fuchsia build failure is because they ingest the libcxx CMakeLists.txt directly, without going through the one in runtimes/. Should we reconsider setting LLVM_THIRD_PARTY_DIR in runtimes/CMakeLists.txt, and, instead, set it separately in libcxx and libc's CMakeLists? or, is the way Fuchsia is ingesting libcxx's CMakeLists incorrect? Comment Actions
That's not an issue with the Fuchsia build, it's an issue with the libFuzzer build which we include in our toolchain build. libFuzzer on Linux and Fuchsia uses a private copy of libc++ and that build still build still goes through libcxx/CMakeLists.txt, see: https://github.com/llvm/llvm-project/blob/01510ac08474a6c4beae033794b71b6b5ab1e89d/compiler-rt/lib/fuzzer/CMakeLists.txt#L160 We need to change it to use runtimes/CMakeLists.txt instead. Comment Actions This broke building for MinGW, due to a local change (from rG948ce4e6edec6ad3cdf1911fc3e8e9569140d4ff that never was upstreamed) that was dropped as part of the move. I've submitted that fix upstream now in https://github.com/google/benchmark/pull/1302, and I filed D115434 to get it applied on the new location. This also showed another brittleness in the benchmark library, as it's built with -Werror by default (in non-MSVC builds). As the benchmark library is built by default when building all of LLVM, building it with -Werror feels excessive and brittle. -Werror was commented out in the original import of the benchmark library in rG0addd170ab0880941fa4089c2717f3f3a0e4e25a, but wasn't commented out in the copy in libcxx (which wasn't built by default, at least not as widely, and thus the issue wasn't noticed there). mtrofin added a child revision: D115434: [benchmark] Reapply fix for -Wcovered-switch-default warning.Dec 9 2021, 7:36 AM Large DiffThis large diff affects 397 files. Files without inline comments have been collapsed. Expand All Files
Revision Contents
Diff 385507 llvm/CMakeLists.txt
llvm/cmake/modules/AddLLVM.cmake
llvm/utils/benchmark/include/benchmark/benchmark.h
runtimes/CMakeLists.txt
|
This is entirely new behavior, and I don't understand why we're doing this as part of this patch. IMO this patch should only be moving stuff around, period. If we then want to start considering the third-party projects as a "project" we can enable with LLVM_ENABLE_PROJECTS, we should tackle it as a separate effort (and frankly it's not clear to me that would fly).
Also, when you make changes like this as part of a much larger change, it is often a good idea to call them out to make sure reviewers see them.