This is an archive of the discontinued LLVM Phabricator instance.

[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:

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

Diff Detail

Event Timeline

mtrofin created this revision.Oct 18 2021, 9:12 AM
mtrofin requested review of this revision.Oct 18 2021, 9:12 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
mtrofin edited the summary of this revision. (Show Details)Oct 18 2021, 9:12 AM
ldionne requested changes to this revision.Oct 18 2021, 1:08 PM

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

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

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

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

@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?

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.

mtrofin updated this revision to Diff 384119.EditedNov 2 2021, 8:27 AM

moved to third_party

since the change is large because of the moved files, the important changes are in:

llvm/CMakeLists.txt
third_party/CMakeLists.txt (added)
llvm/cmake/modules/AddLLVM.cmake
libcxx/benchmarks/CMakeLists.txt

mtrofin edited the summary of this revision. (Show Details)Nov 2 2021, 8:28 AM
ldionne accepted this revision.Nov 3 2021, 9:54 AM

@ldionne What is your concrete proposal? Requiring Google Benchmark pre-installed on the host?

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.

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

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.

I don't think we should reject patches that just follow the status quo without having a plan what the future direction should be.

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

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:
[...]

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.

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:
[...]

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.

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?

mtrofin updated this revision to Diff 385507.Nov 8 2021, 8:19 AM

added def for LLVM_THIRD_PARTY_DIR in runtimes

Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2021, 8:19 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
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

@ldionne - is there anything else I need to do to this change? Thanks!

ldionne requested changes to this revision.Nov 9 2021, 11:47 AM

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.

llvm/CMakeLists.txt
70

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.

1190–1200

This is the minimal change. Just move stuff around and point to the new directory from llvm/ and from runtimes/ + libcxx/benchmarks. Everything else should be in a different patch since that's a meaningful change.

runtimes/CMakeLists.txt
39

Why the if (NOT DEFINED LLVM_THIRD_PARTY_DIR)? If we can set it unconditionally, we should because it avoid giving the impression that folks can set it however they want.

This revision now requires changes to proceed.Nov 9 2021, 11:47 AM
mtrofin updated this revision to Diff 385986.Nov 9 2021, 2:54 PM
mtrofin marked 3 inline comments as done.

no need to make third-party a project

llvm/CMakeLists.txt
70

Huh, I thought this was the approach to follow for adding subprojects that are peers (dir structure) to llvm. Probably got confused by 'peer dirs' and 'projects' as identical concepts - in any case, fixed.

1190–1200

I assume the 'everything else' refers to the making of third-party as a project, which is moot now?

Note that D113503 has landed now, so you should rebase onto main.

llvm/CMakeLists.txt
1004

I don't understand why this is needed.

llvm/cmake/modules/AddLLVM.cmake
1538

I don't understand why this is needed.

mtrofin marked 2 inline comments as done.Nov 15 2021, 7:53 AM
mtrofin added inline comments.
llvm/CMakeLists.txt
1004

not needed

llvm/cmake/modules/AddLLVM.cmake
1538

I thought it was needed to make sure the library is built, but I suppose the line below addresses that.

mtrofin updated this revision to Diff 387273.Nov 15 2021, 8:02 AM
mtrofin marked 2 inline comments as done.

removed unneeded lines

ldionne accepted this revision.Nov 16 2021, 8:50 AM

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
This revision was automatically updated to reflect the committed changes.

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.

llvm/utils/benchmark/include/benchmark/benchmark.h
1236

The definition of this struct is not the same as the one in the libcxx copy.

Oh! Huh, missed that. Probably my ninja check-all didn’t build the
benchmarks. I’ll patch shortly. Sorry for the lapse.

haowei added a subscriber: haowei.Nov 16 2021, 4:24 PM

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
Could you revert the change if the fix will take a while? Thanks.

Oh! Huh, missed that. Probably my ninja check-all didn’t build the
benchmarks. I’ll patch shortly. Sorry for the lapse.

I have already fixed the libc build.

Oh! Huh, missed that. Probably my ninja check-all didn’t build the
benchmarks. I’ll patch shortly. Sorry for the lapse.

I have already fixed the libc build.

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

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
Could you revert the change if the fix will take a while? Thanks.

@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?

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
Could you revert the change if the fix will take a while? Thanks.

@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?

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
https://github.com/llvm/llvm-project/blob/01510ac08474a6c4beae033794b71b6b5ab1e89d/compiler-rt/cmake/Modules/AddCompilerRT.cmake#L546
https://github.com/llvm/llvm-project/blob/01510ac08474a6c4beae033794b71b6b5ab1e89d/compiler-rt/cmake/Modules/CustomLibcxx/CMakeLists.txt

We need to change it to use runtimes/CMakeLists.txt instead.

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