-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[ORC-RT] Initial check-in for a new, top-level ORC runtime project. #113499
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Includes CMake files and a placeholder header, library, and tool. See discussion at https://discourse.llvm.org/t/rfc-move-orc-executor-support-into-top-level-project/81049
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions h,cpp -- orc-rt/include/orc-rt-c/orc-rt.h orc-rt/lib/executor/orc-rt-executor.cpp orc-rt/tools/orc-executor/orc-executor.cpp orc-rt/unittests/init.cpp View the diff from clang-format here.diff --git a/orc-rt/unittests/init.cpp b/orc-rt/unittests/init.cpp
index 24f12127f..d98103a5a 100644
--- a/orc-rt/unittests/init.cpp
+++ b/orc-rt/unittests/init.cpp
@@ -1,6 +1,3 @@
#include "gtest/gtest.h"
-
-TEST(TEST, emptyFuncs) {
- ASSERT_EQ(True, False);
-}
+TEST(TEST, emptyFuncs) { ASSERT_EQ(True, False); }
|
A couple of initial questions:
|
orc-rt/include/CMakeLists.txt
Outdated
target_sources(orc-rt-headers | ||
INTERFACE FILE_SET HEADERS | ||
BASE_DIRS ${CMAKE_CURRENT_SOURCE_DIR} | ||
FILES ${files} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File Sets were introduced in CMake 3.23 but LLVM currently requires 3.20 as the minimum version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for spotting that! I've switched to install (DIRECTORY ...
in the latest commits -- Is that a reasonable solution for CMake 3.20?
@petrhosek @etcwilde I've added some skeleton docs, plus LICENSE.TXT and CREDITS.TXT. Everything works when building for host with a simple configuration (at least on Darwin/arm64): % xcrun cmake -GNinja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_INSTALL_PREFIX="`pwd`/install" -DLLVM_ENABLE_RUNTIMES=orc-rt /path/to/llvm-project/runtimes
% ninja install My next question is how this should be extended to support building for multiple targets, and whether this is something that should be tackled in the initial commit or in a follow-up? |
You can test this locally with the following command:darker --check --diff -r HEAD~1...HEAD orc-rt/docs/conf.py orc-rt/test/lit.cfg.py orc-rt/test/unit/lit.cfg.py View the diff from darker here.--- test/lit.cfg.py 2025-08-05 09:04:38.000000 +0000
+++ test/lit.cfg.py 2025-08-05 09:07:47.044315 +0000
@@ -41,11 +41,12 @@
# test_exec_root: The root path where tests should be run.
config.test_exec_root = os.path.join(config.orcrt_obj_root, "test")
llvm_config.with_environment(
"PATH",
os.path.join(config.orcrt_obj_root, "tools", "orc-executor"),
- append_path=True)
+ append_path=True,
+)
config.substitutions.append(("%PATH%", config.environment["PATH"]))
# config.substitutions.append(("%shlibext", config.llvm_shlib_ext))
config.substitutions.append(("%llvm_src_root", config.llvm_src_root))
# config.substitutions.append(("%host_cxx", config.host_cxx))
# config.substitutions.append(("%host_cc", config.host_cc))
|
@petrhosek @llvm-beanz I'm still trying to figure out how to do a multi-target build. I tried:
but that yields a warning about Are multi-target builds possible when using |
Having done a bit more reading, and chatting with @etcwilde, it looks like |
Runtimes when included as the top-level CMake file is a single configuration, when included from LLVM it is multi-configuration. LLVM's build manages the multi-invocation. The Fuchsia toolchains build multiple targets using the runtimes build, and is a good example for the setup.
The existing runtimes build is designed to only support building the runtimes with the just-built compiler, so it does not support multi configuration building with the host compiler. |
Is there any desire to support multi-configuration builds with the host compiler? In the ORC runtime's case I'd expect this to work pretty well (I expect most if not all of the code to be standard C++ and a little custom assembly), but I know in general runtimes are often tightly coupled to their associated compilers. |
@llvm-beanz @petrhosek I'm just getting back to this. I've hit a sticking point on Darwin: If I want to do a multiconfig build and take the compiler-rt approach: % xcrun cmake -GNinja -DCMAKE_BUILD_TYPE=Debug \
-DLLVM_ENABLE_PROJECTS="llvm;clang" \
-DLLVM_ENABLE_RUNTIMES="orc-rt" \
-DDARWIN_macosx_ARCHS="arm64;x86-64" \
/path/to/llvm-project/llvm I still only get a single-architecture archive, presumably because the CMake in this PR doesn't speak Apple-multiarch (by design). On the other hand if I try the Linux approach and specify: % xcrun cmake -GNinja -DCMAKE_BUILD_TYPE=Debug \
-DLLVM_ENABLE_PROJECTS="llvm;clang" \
-DLLVM_ENABLE_RUNTIMES="orc-rt" \
-DLLVM_RUNTIME_TARGETS="arm64-apple-darwin;x86_64-apple-darwin" \
/Users/lhames/Projects/llvm/llvm-github/llvm I get an error at config time:
Passing If building compiler-rt is required for multi-config builds I guess we're going to need some sort of interim solution for Darwin that allows existing projects (compiler-rt in particular) to keep building, while allowing new projects to use |
The compiler-rt Darwin build is a sin of mine that should not be repeated. Unfortunately it isn't something someone outside Apple can really fix because Apple has a hard dependency on it for Clang builds. The only reason why the compiler-rt build generates fat archives was because I was required by management to ensure that the Apple Clang builds with CMake produced identical outputs to the autoconf builds (a ridiculous request that I'm still bitter about almost a decade later). I always felt that it was probably better to have darwin build thin-libraries and even put them in separate directories structured the way the Linux toolchain does. That would allow simplifying a lot of logic in the darwin driver too. Since the driver always generates single-architecture compile and link commands and uses lipo to merge objects when building fat binaries, there is no reason that fat archives are strictly needed. |
I discussed this with my group and we agreed that we'd like the ORC runtime to build as you've suggested. There were no objections to moving more Apple projects in this direction, but I didn't canvas opinions more widely inside Apple. ;) Do you think it'd be reasonable / possible to add some CMake allow a linux-style ORC runtime to coexist with an Apple legacy-style build of compiler-rt (which seems to be a prerequisite for a multiconfig build)? At the very least it seems like that'll require isolating compiler-rt from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few CMake nits here.
Co-authored-by: Ben Boeckel <mathstuf@users.noreply.github.com>
orc-rt/include/CMakeLists.txt
Outdated
set_property(TARGET orc-rt-headers | ||
PROPERTY PUBLIC_HEADER ${ORC_RT_HEADERS} | ||
) | ||
install(DIRECTORY ./ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer using a separate install
rule for each header in ORC_RT_HEADERS
rather than a directory rule. This is is more explicit and matches what we use elsewhere in LLVM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that using FILE_SET
specification makes it a lot easier to manage this, but that is CMake 3.23+.
set(ORC_RT_HEADERS | ||
orc-rt-c/orc-rt.h | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should also be a custom command that copies every header in ORC_RT_HEADERS
to output directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm…what's the reason here?
orc-rt/include/CMakeLists.txt
Outdated
install(TARGETS orc-rt-headers | ||
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/ | ||
FILES_MATCHING PATTERN "*.h" | ||
COMPONENT OrcRT_Development |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't install headers unless you use FILE_SET
(and then you need to install the file set).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that the PUBLIC_HEADER
property set on orc-rt-headers
above would cause install(TARGET ...)
to install the headers:
set_property(TARGET orc-rt-headers
PROPERTY PUBLIC_HEADER ${ORC_RT_HEADERS}
)
If I configure and build on linux with
% cmake -GNinja -DCMAKE_INSTALL_PREFIX=/path/to/install -DCMAKE_BUILD_TYPE=Debug -DLLVM_ENABLE_RUNTIMES=orc-rt /path/to/llvm-project/runtimes
% ninja install
I see the headers in the expected location. Am I relying on non-portable behavior here though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh! Yes, PUBLIC_HEADER
should work; I had forgotten about this mechanism. I believe it flattens the hierarchy, but maybe that's not the case.
@lhames, could you rebase this PR. If we get a green ci I think we can move forward. |
ping... |
@vgvassilev Thanks for the ping! @jaredwy got tests working on Darwin and Linux. @llvm-beanz @petrhosek -- any comments on whether we're approaching the test infrastructure the right way here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request has been under multiple review iterations and I'd like to thank everybody involved. The downstream clients of clang-repl need this and I'd like to move forward because it also blocks a google summer of code contributor of the llvm mentoring org.
Thank you, @lhames! LGTM.
Perhaps, @llvm-beanz and @petrhosek can take another look maybe as part of a post-commit review if there is something missing.
Includes CMake files and a placeholder header, library, and tool.
See discussion at
https://discourse.llvm.org/t/rfc-move-orc-executor-support-into-top-level-project/81049
The goal for this initial checkin is a stub runtime library with modern CMake idioms that other projects could use as a guide / template. The ORC details shouldn't matter much here, it's the build system and integration with the rest of LLVM that I want to get right.
Eventually we will move the ORC runtime code from its current home in
compiler-rt
to this new top-level project.