Skip to content

[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

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

lhames
Copy link
Contributor

@lhames lhames commented Oct 23, 2024

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.

@lhames lhames requested a review from a team as a code owner October 23, 2024 21:31
@llvmbot llvmbot added the cmake Build system in general and CMake in particular label Oct 23, 2024
Copy link

github-actions bot commented Oct 23, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

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); }

@lhames
Copy link
Contributor Author

lhames commented Oct 27, 2024

A couple of initial questions:

  1. How should the runtime inherit compile options (e.g. -fno-exceptions and -fno-rtti)? Should it inherit them at all, or should it have its own defaults that can be overridden just for this runtime?

  2. How should include directories and installation of headers be managed? For now I've created an interface library with an install command, but that was largely cargo-culted from Stack Overflow. If there's a better way to do this please let me know!

Comment on lines 10 to 14
target_sources(orc-rt-headers
INTERFACE FILE_SET HEADERS
BASE_DIRS ${CMAKE_CURRENT_SOURCE_DIR}
FILES ${files}
)
Copy link
Member

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.

Copy link
Contributor Author

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?

@lhames
Copy link
Contributor Author

lhames commented Oct 31, 2024

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

Copy link

github-actions bot commented Oct 31, 2024

⚠️ Python code formatter, darker found issues in your code. ⚠️

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

@lhames
Copy link
Contributor Author

lhames commented Nov 6, 2024

@petrhosek @llvm-beanz I'm still trying to figure out how to do a multi-target build. I tried:

% cmake -GNinja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_INSTALL_PREFIX="`pwd`/install" -DLLVM_ENABLE_RUNTIMES=orc-rt --DLLVM_RUNTIME_TARGETS="aarch64-linux;x86_64-linux" /path/to/llvm-project/runtimes

but that yields a warning about LLVM_RUNTIME_TARGETS being unused.

Are multi-target builds possible when using llvm-project/runtimes as the source directory? Are there any other options that I need to pass?

@lhames
Copy link
Contributor Author

lhames commented Nov 6, 2024

Are multi-target builds possible when using llvm-project/runtimes as the source directory? Are there any other options that I need to pass?

Having done a bit more reading, and chatting with @etcwilde, it looks like llvm-project/runtimes is for single target builds and we expect some higher level script to invoke this for each target. Using llvm-project/llvm as the source project gets us some kind of multi-target support, but I'm struggling to hold it right: Ideally I want to be able to do a multi-target build of orc-rt with either the host compiler or the just-built compiler, depending on config. Is that doable with the existing CMake machinery?

@llvm-beanz
Copy link
Collaborator

Having done a bit more reading, and chatting with @etcwilde, it looks like llvm-project/runtimes is for single target builds and we expect some higher level script to invoke this for each target.

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.

Using llvm-project/llvm as the source project gets us some kind of multi-target support, but I'm struggling to hold it right: Ideally I want to be able to do a multi-target build of orc-rt with either the host compiler or the just-built compiler, depending on config. Is that doable with the existing CMake machinery?

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.

@lhames
Copy link
Contributor Author

lhames commented Nov 13, 2024

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.

@lhames
Copy link
Contributor Author

lhames commented Jan 23, 2025

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

CMake Error at runtimes/CMakeLists.txt:64 (message):
  compiler-rt for Darwin builds for all platforms and architectures using a
  single configuration.  Specify only a single darwin triple (e.g.
  x86_64-apple-darwin) in your targets list (and not a triple for a specific
  platform such as macos).  You can use variables such as
  COMPILER_RT_ENABLE_IOS and DARWIN_ios_ARCHS to control the specific
  platforms and architectures to build.  Set RUNTIMES_BUILD_ALLOW_DARWIN to
  allow a single darwin triple.
Call Stack (most recent call first):
  runtimes/CMakeLists.txt:623 (check_apple_target)

Passing -DRUNTIMES_BUILD_ALLOW_DARWIN does allow a single triple to be specified, but doesn't allow multi-config.

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 -DLLVM_RUNTIME_TARGETS on Apple. Would it make sense to add an LLVM_RUNTIMES_ISOLATE_APPLE_LEGACY_BUILDS option that resets RUNTIMES_BUILD_ALLOW_DARWIN (and any other arguments that aren't expected to be used for Apple-style builds) for Apple-style subprojects?

@llvm-beanz
Copy link
Collaborator

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.

@lhames
Copy link
Contributor Author

lhames commented Jan 24, 2025

@llvm-beanz

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 LLVM_RUNTIME_TARGETS when the ORC runtime is being built.

Copy link
Contributor

@mathstuf mathstuf left a 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.

set_property(TARGET orc-rt-headers
PROPERTY PUBLIC_HEADER ${ORC_RT_HEADERS}
)
install(DIRECTORY ./
Copy link
Member

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.

Copy link
Contributor

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
)

Copy link
Member

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.

Copy link
Contributor

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?

Comment on lines 14 to 16
install(TARGETS orc-rt-headers
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/
FILES_MATCHING PATTERN "*.h"
COMPONENT OrcRT_Development
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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.

@vgvassilev
Copy link
Contributor

@lhames, could you rebase this PR. If we get a green ci I think we can move forward.

@vgvassilev
Copy link
Contributor

@lhames, could you rebase this PR. If we get a green ci I think we can move forward.

ping...

@lhames
Copy link
Contributor Author

lhames commented Aug 3, 2025

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

Copy link
Contributor

@vgvassilev vgvassilev left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake Build system in general and CMake in particular
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants