-
Notifications
You must be signed in to change notification settings - Fork 31
Adds Out-Of-Process JIT execution for CppInterOp #635
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?
Changes from all commits
5a1cac3
eea2a1c
3ac5647
cc3f601
4396765
5536edf
fac8d38
9510241
34d5670
e562d13
13a0caf
43220d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,6 +49,10 @@ runs: | |
git apply -v ../patches/llvm/clang${{ matrix.clang-runtime }}-*.patch | ||
echo "Apply clang${{ matrix.clang-runtime }}-*.patch patches:" | ||
fi | ||
if [[ "${{ matrix.oop-jit }}" == "On" ]]; then | ||
git apply -v ../patches/llvm/clang20-2-out-of-process-jit-execution.patch | ||
echo "Apply out-of-process-jit-execution.patch:" | ||
fi | ||
cd build | ||
cmake -DLLVM_ENABLE_PROJECTS="${{ matrix.llvm_enable_projects}}" \ | ||
-DLLVM_TARGETS_TO_BUILD="${{ matrix.llvm_targets_to_build }}" \ | ||
|
@@ -64,6 +68,10 @@ runs: | |
-DLLVM_INCLUDE_TESTS=OFF \ | ||
../llvm | ||
ninja clang clangInterpreter clangStaticAnalyzerCore -j ${{ env.ncpus }} | ||
if [[ "${{ matrix.oop-jit }}" == "On" ]]; then | ||
if [ "${{ matrix.os }}}" == "macos"* ]; then SUFFIX="_osx"; fi | ||
ninja clang_repl llvm-jitlink-executor orc_rt${SUFFIX} -j ${{ env.ncpus }} | ||
fi | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we have target name for orc_rt difference for MacOS? The target name should be independent of the system your building on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is different in LLVM itself. I guess it might have to do with cross-compiling. |
||
cd ./tools/ | ||
rm -rf $(find . -maxdepth 1 ! -name "clang" ! -name ".") | ||
cd .. | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -310,6 +310,16 @@ include_directories(SYSTEM ${LLVM_INCLUDE_DIRS}) | |||||||||
separate_arguments(LLVM_DEFINITIONS_LIST NATIVE_COMMAND ${LLVM_DEFINITIONS}) | ||||||||||
add_definitions(${LLVM_DEFINITIONS_LIST}) | ||||||||||
|
||||||||||
if(CPPINTEROP_WITH_OOP_JIT) | ||||||||||
kr-2003 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
if(WIN32 OR EMSCRIPTEN) | ||||||||||
message(FATAL_ERROR "CPPINTEROP_WITH_OOP_JIT is not supported on Windows or Emscripten platforms.") | ||||||||||
endif() | ||||||||||
add_definitions(-DCPPINTEROP_WITH_OOP_JIT) | ||||||||||
endif() | ||||||||||
|
||||||||||
string(REGEX REPLACE "/build/lib/cmake/llvm$" "" LLVM_SOURCE_DIR "${LLVM_DIR}") | ||||||||||
add_definitions(-DLLVM_SOURCE_DIR="${LLVM_SOURCE_DIR}") | ||||||||||
Comment on lines
+320
to
+321
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I guess is a better name. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LLVM_BINARY_DIR is already an env variable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we reuse |
||||||||||
|
||||||||||
# If the llvm sources are present add them with higher priority. | ||||||||||
if (LLVM_BUILD_MAIN_SRC_DIR) | ||||||||||
# LLVM_INCLUDE_DIRS contains the include paths to both LLVM's source and | ||||||||||
|
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.
We should apply this patch regardless of whether matrix.oop-jit=on. If you only apply it if CPPINTEROP_WITH_OOP_JIT=ON , this would imply if you apply the patch, DCPPINTEROP_WITH_OOP_JIT=OFF, then somehow something will go wrong.
Also, why are we not applying this patch on Windows, and testing CPPINTEROP_WITH_OOP_JIT=ON/OFF?
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.
Ok. The thing is, I would want CppInterOp to be compatible with upstream LLVM as well as the patched LLVM. Therefore, I was thinking about having 2 different CI workflows for version 20 with OOP-JIT and without OOP-JIT.
OOP-JIT is not supported on Windows.
Ideally, this is temporary till the patch is directly merged into LLVM and a new version is released.
Uh oh!
There was an error while loading. Please reload this page.
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 could be wrong, and @vgvassilev can correct me if I am wrong, but for such large patches, we have had them merged into llvm first, before applying them to previous versions in CppInterOp.
You could have a new llvm cache like you want. One where you build OOP-JIT and without OOP-JIT. This could only really be done after I find a way to make space in the cache, since we currently are unlikely to have enough space for all these new llvm cache builds. I have an idea on how to free up some space, and just need to find some time to do. I will try and do it tomorrow, as I am busy today.