This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Support runtimes targets without specifying triple
ClosedPublic

Authored by phosek on Jan 13 2022, 4:53 PM.

Details

Summary

Currently, for BUILTIN_TARGETS and RUNTIME_TARGETS you can either use
the special "default" value, or a target triple.

For the "default" value, we don't set any target triple and passthrough
a subset of CMake variables into the subbuild. This is typically used
on Darwin where we build universal binaries and a single target triple
therefore isn't sufficient.

For the target triple value, you can set arbitrary CMake variables
through RUNTIMES_<target>_<variable>, but we always set target triple
to <target>. This gives more flexibility, because we can precisely
control what variables are set in the subbuild, but is unsuitable for
platforms like Darwin.

To address this, we would like to introduce a third option which is
similar to the second option, except we won't set target triple in
the subbuild (you can always do so yourself by setting the appropriate
CMake variable, e.g. RUNTIMES_<name>_CMAKE_C_COMPILER_TARGET=<triple>).

This change is a first step in that direction, by eliminating the support
of target triples from builtin_register_target and runtime_register_target
helper functions.

Diff Detail

Event Timeline

phosek created this revision.Jan 13 2022, 4:53 PM
phosek requested review of this revision.Jan 13 2022, 4:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2022, 4:53 PM
phosek edited the summary of this revision. (Show Details)Jan 13 2022, 4:53 PM

I don't understand the motivation for this change.

The Darwin compiler-rt support works by overriding the OSX_ARCHITECTURES target property (which makes CMake pass -arch flags), and emptying CMAKE_OSX_DEPLOYMENT_TARGET and passing its own -m*-version-min flags. The Clang driver seems to have some special support where if your triple is x86_64-apple-darwin (I assume other architectures would work too, but I haven't checked), it'll override the architecture and the deployment target based on the -arch and -m*-version-min flags, which means the -target is effectively a no-op in this case (besides enabling the overwriting based on those other flags). The result is that you can add x86_64-apple-darwin to your LLVM_BUILTIN_TARGETS and LLVM_RUNTIME_TARGETS, and then control the actual platforms and architectures via the appropriate CMake variables for that target (COMPILER_RT_ENABLE_<PLATFORM>, DARWIN_<PLATFORM>_BUILTIN_ARCHS, etc.), which is what I set up in D86313, and what we use internally. Does that not work for your use case?

phosek added a comment.EditedJan 13 2022, 6:39 PM

I don't understand the motivation for this change.

The Darwin compiler-rt support works by overriding the OSX_ARCHITECTURES target property (which makes CMake pass -arch flags), and emptying CMAKE_OSX_DEPLOYMENT_TARGET and passing its own -m*-version-min flags. The Clang driver seems to have some special support where if your triple is x86_64-apple-darwin (I assume other architectures would work too, but I haven't checked), it'll override the architecture and the deployment target based on the -arch and -m*-version-min flags, which means the -target is effectively a no-op in this case (besides enabling the overwriting based on those other flags). The result is that you can add x86_64-apple-darwin to your LLVM_BUILTIN_TARGETS and LLVM_RUNTIME_TARGETS, and then control the actual platforms and architectures via the appropriate CMake variables for that target (COMPILER_RT_ENABLE_<PLATFORM>, DARWIN_<PLATFORM>_BUILTIN_ARCHS, etc.), which is what I set up in D86313, and what we use internally. Does that not work for your use case?

I'm mostly concerned about the potential confusion. Specifically, I'd like to start building libunwind, libc++abi, libc++ as a universal library for arm64 and x86_64 in our toolchain and I'd like to use CMAKE_OSX_ARCHITECTURES for that purpose. I could do what you've suggested, it's just that seeing something like:

RUNTIMES_x86_64-apple-darwin_CMAKE_OSX_ARCHITECTURES=arm64;x64

is in my opinion more confusing than something like:

RUNTIMES_darwin_CMAKE_OSX_ARCHITECTURES=arm64;x64

which is what I'm trying to achieve. I'm interested in your experience and thoughts on this, especially if there are other alternatives you think I should consider.

Another point I'd also add is that using CMAKE_{ASM,C,CXX}_COMPILER_TARGET as we do today isn't always sufficient. We ran into this recently on Windows. The problem is that CMake only processes CMAKE_{ASM,C,CXX}_COMPILER_TARGET after it does compiler detection, but in the case of Clang, --target= option changes the compiler behavior which can throw CMake compiler detection off. Specifically on Windows, CMake would detect Clang as "clang-cl in GNU compatibility mode" and to workaround it, we had to start passing --target= manually via CMAKE_{ASM,C,CXX}_FLAGS so the target triple handling runtimes build does right now looses some of its value. I'd more inclined to just deprecate it and require users to set those flags manually, but I'm not sure if that's feasible at this point since it'd be a breaking change.

smeenai accepted this revision.Apr 28 2022, 1:53 PM

Sorry, this fell off my radar! Your reasoning makes sense to me, and in particular, I'd forgotten that only compiler-t has the Darwin multi-arch setup (and you'd still need to handle libc++/libc++abi/libunwind differently).

llvm/runtimes/CMakeLists.txt
256

This part of the multilib setup has always confused me. Is there only ever going to be one extra name (as is the case in this diff)? If so, maybe something like BASE_NAME or a different name would convey the intent better?

This revision is now accepted and ready to land.Apr 28 2022, 1:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2022, 1:53 PM
phosek updated this revision to Diff 494851.Feb 4 2023, 2:43 PM
phosek marked an inline comment as done.
This revision was landed with ongoing or failed builds.Feb 4 2023, 3:00 PM
This revision was automatically updated to reflect the committed changes.
phosek reopened this revision.Feb 5 2023, 6:53 PM
phosek updated this revision to Diff 494977.
This revision is now accepted and ready to land.Feb 5 2023, 6:54 PM
phosek updated this revision to Diff 496718.Feb 11 2023, 2:26 PM
phosek updated this revision to Diff 510946.Apr 4 2023, 3:18 PM
This revision was landed with ongoing or failed builds.Apr 5 2023, 1:12 PM
This revision was automatically updated to reflect the committed changes.

This was breaking our internal builds. rG03610cdeb979 should fix it.