You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "niyue (via GitHub)" <gi...@apache.org> on 2023/12/06 09:38:13 UTC
[PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
niyue opened a new pull request, #39098:
URL: https://github.com/apache/arrow/pull/39098
### Rationale for this change
Gandiva currently employs MCJIT as its internal JIT engine. However, LLVM has introduced a newer JIT API known as ORC v2/LLJIT since LLVM 7.0, and it has several advantage over MCJIT, in particular, MCJIT is not actively maintained, and is slated for eventual deprecation and removal.
### What changes are included in this PR?
This PR replaces the MCJIT JIT engine with the ORC v2 engine, using the `LLJIT` API.
### Are these changes tested?
Yes, they are covered by existing unit tests
### Are there any user-facing changes?
* `Configuration` class has a new option called `needs_ir_dumping`. If users would like to call `DumpIR` API of `Projector` and `Filter`, they have to set the `needs_ir_dumping` option first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1417033097
##########
cpp/src/gandiva/engine_llvm_test.cc:
##########
@@ -24,14 +24,14 @@
namespace gandiva {
-typedef int64_t (*add_vector_func_t)(int64_t* data, int n);
+using add_vector_func_t = int64_t (*)(int64_t*, int);
class TestEngine : public ::testing::Test {
protected:
- std::string BuildVecAdd(Engine* engine) {
- auto types = engine->types();
- llvm::IRBuilder<>* builder = engine->ir_builder();
- llvm::LLVMContext* context = engine->context();
+ std::string BuildVecAdd(Engine* gdv_engine) {
Review Comment:
The `engine` param is renamed to `gdv_engine` since it shadows the `engine` member variable in this class which introduces unnecessary confusion here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1433607844
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,136 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
Review Comment:
Can we require `error_context`?
```suggestion
const std::string& error_context ) {
```
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,135 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
+ if (!expected) {
+ return Status::CodeGenError(error_context, llvm::toString(expected.takeError()));
+ }
+ return std::move(expected.get());
+}
+
+Result<llvm::orc::JITTargetMachineBuilder> GetTargetMachineBuilder(
+ const Configuration& conf) {
+ llvm::orc::JITTargetMachineBuilder jtmb(
+ (llvm::Triple(llvm::sys::getDefaultTargetTriple())));
+ if (conf.target_host_cpu()) {
+ jtmb.setCPU(cpu_name.str());
+ jtmb.addFeatures(cpu_attrs);
+ }
+ auto const opt_level =
+ conf.optimize() ? llvm::CodeGenOpt::Aggressive : llvm::CodeGenOpt::None;
+ jtmb.setCodeGenOptLevel(opt_level);
+ return jtmb;
+}
+
+std::string GetModuleIR(const llvm::Module& module) {
+ std::string ir;
+ llvm::raw_string_ostream stream(ir);
+ module.print(stream, nullptr);
+ return ir;
+}
+
+void AddAbsoluteSymbol(llvm::orc::LLJIT& lljit, const std::string& name,
+ void* function_ptr) {
+ llvm::orc::MangleAndInterner mangle(lljit.getExecutionSession(), lljit.getDataLayout());
+
+ // https://github.com/llvm/llvm-project/commit/8b1771bd9f304be39d4dcbdcccedb6d3bcd18200#diff-77984a824d9182e5c67a481740f3bc5da78d5bd4cf6e1716a083ddb30a4a4931
+ // LLVM 17 introduced ExecutorSymbolDef and move most of ORC APIs to ExecutorAddr
+#if LLVM_VERSION_MAJOR >= 17
+ llvm::orc::ExecutorSymbolDef symbol(
+ llvm::orc::ExecutorAddr(reinterpret_cast<uint64_t>(function_ptr)),
+ llvm::JITSymbolFlags::Exported);
+#else
+ llvm::JITEvaluatedSymbol symbol(reinterpret_cast<llvm::JITTargetAddress>(function_ptr),
+ llvm::JITSymbolFlags::Exported);
+#endif
+
+ auto error = lljit.getMainJITDylib().define(
+ llvm::orc::absoluteSymbols({{mangle(name), symbol}}));
+ llvm::cantFail(std::move(error));
Review Comment:
@niyue It seems that this is not changed yet. You might forget to push your change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1420656676
##########
cpp/src/gandiva/llvm_generator_test.cc:
##########
@@ -100,7 +98,9 @@ TEST_F(TestLLVMGenerator, TestAdd) {
auto field_sum = std::make_shared<arrow::Field>("out", arrow::int32());
auto desc_sum = annotator.CheckAndAddInputFieldDescriptor(field_sum);
- std::string fn_name = "codegen";
+ // LLVM 10 doesn't like the expr function name to be the same as the module name when
+ // LLJIT is used
+ std::string fn_name = "llvm_gen_test_add_expr";
Review Comment:
There is a weird behavior of LLVM LLJIT. The LLVM module is created with name `codegen`, if later a function is created with the same name `codegen`, it can be successfully created, when looking up its function pointer, a successful symbol will be returned, however, the function address inside the symbol is 0, making it a nullptr and cannot be used.
* This issue happened for LLVM 10 (not sure if it is Linux specific), and the same code doesn't have such an issue when using LLVM 14.0.6 on macOS
* This issue is specific for ORC v2/LLJIT, and MCJIT doesn't have such an issue
To workaround this issue, I have to:
1) rename the function name in the test case to be more specific and avoid conflict
2) rename the LLVM module name to be more specific and add some random suffix (the object's address) to avoid conflict
UPDATE:
I reported a bug to LLVM, https://github.com/llvm/llvm-project/issues/74903
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1428845846
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,135 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
+ if (!expected) {
+ return Status::CodeGenError(error_context, llvm::toString(expected.takeError()));
+ }
+ return std::move(expected.get());
+}
+
+Result<llvm::orc::JITTargetMachineBuilder> GetTargetMachineBuilder(
Review Comment:
Sure. Renamed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1428847690
##########
cpp/src/gandiva/llvm_generator_test.cc:
##########
@@ -110,8 +110,10 @@ TEST_F(TestLLVMGenerator, TestAdd) {
auto ir = generator->engine_->DumpIR();
EXPECT_THAT(ir, testing::HasSubstr("vector.body"));
- EvalFunc eval_func = (EvalFunc)generator->engine_->CompiledFunction(fn_name);
+ EXPECT_OK_AND_ASSIGN(auto fn_ptr, generator->engine_->CompiledFunction(fn_name));
+ EXPECT_NE(fn_ptr, nullptr);
Review Comment:
Updated. I don't realize their difference previously. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1847895161
Could you use `LLVM=14 CLANG_TOOLS=14 UBUNTU=22.04 archery docker run ubuntu-cpp-sanitizer` for `C++ / AMD64 Ubuntu 22.04 C++ ASAN UBSAN`?
See also: https://github.com/apache/arrow/actions/runs/7143576730/job/19455365682?pr=39098#step:6:4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1434711551
##########
cpp/src/gandiva/engine.h:
##########
@@ -30,23 +35,34 @@
#include "gandiva/llvm_types.h"
#include "gandiva/visibility.h"
+namespace llvm::orc {
+class LLJIT;
+} // namespace llvm::orc
+
namespace gandiva {
/// \brief LLVM Execution engine wrapper.
class GANDIVA_EXPORT Engine {
public:
+ ~Engine();
llvm::LLVMContext* context() { return context_.get(); }
llvm::IRBuilder<>* ir_builder() { return ir_builder_.get(); }
LLVMTypes* types() { return &types_; }
- llvm::Module* module() { return module_; }
+
+ /// Retrieve LLVM module in the engine.
+ /// This should only be called before `FinalizeModule` is called
+ llvm::Module* module();
/// Factory method to create and initialize the engine object.
///
/// \param[in] config the engine configuration
/// \param[in] cached flag to mark if the module is already compiled and cached
- /// \param[out] engine the created engine
- static Status Make(const std::shared_ptr<Configuration>& config, bool cached,
- std::unique_ptr<Engine>* engine);
+ /// \param[in] object_cache an optional object_cache used for building the module
+ /// \return arrow::Result containing the created engine
+ static Result<std::unique_ptr<Engine>> Make(
Review Comment:
I commented here about this change (https://github.com/apache/arrow/pull/39098#discussion_r1417035796)
LLJIT requires cache related parameter to be passed during JIT instance construction (you can find the details here, https://github.com/apache/arrow/pull/39098/files/b484dfbc293de5d2bca5cf4d294bc5765ff51194#diff-0fbbced7a02cae757d015b7dec201c949bab76bab3c97d12d0a4bdd9712b4e70R212-R224)
So we have to change the `LLVMGenerator`/`Engine` constructor to add the `GandivaCache` as a parameter in order to use LLJIT, because previously there is an `out` parameter in their constructors, it is not practical to add an optional parameter as the last parameter in the constructor, which means we have to change the constructor anyway in this PR, so I change it in a more radical way to take advantage of `arrow::Result`.
I think Gandiva users typically uses `Projector`/`Filter` but do not use `LLVMGenerator` and `Engine` API directly, so it may be okay to consider this is an internal API change. What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1848380367
I spent a day trying to address the last failed test case, but I am still not able to reproduce it yet.
* My local env: macOS 14 + Macbook M1 MAX (Apple Silicon) + Docker Desktop for Mac 4.26.0 (with Rosetta enabled for x86 images)
* I use `LLVM=14 CLANG_TOOLS=14 UBUNTU=22.04 archery docker run ubuntu-cpp-sanitizer` to start the container
* When `ARROW_USE_ASAN` and `ARROW_USE_UBSAN` are set to `ON` (as CI does), I don't see the `JIT session error: Symbols not found: [ atexit ]` and `'_error_or_value29.status()' failed with CodeGenError in Gandiva: Failed to look up function: llvm_gen_test_add_expr error: Failed to materialize symbols: { (main, { ...` error reported by CI, instead, I see some sanitizer issue reported by ASAN and the program is aborted like:
> AddressSanitizer:DEADLYSIGNAL
=================================================================
==39275==ERROR: AddressSanitizer: SEGV on unknown address 0x200071c9c800 (pc 0x555555b9df32 bp 0x5554ffffa610 sp 0x5554ffff9dc8 T0)
==39275==The signal is caused by a READ memory access.
#0 0x555555b9df32 in __asan::QuickCheckForUnpoisonedRegion(unsigned long, unsigned long) asan_interceptors.cpp.o
#1 0x555555ba1970 in __interceptor_memset (/build/cpp/debug/gandiva-projector-test+0x64d970) (BuildId: 39a4e12fe1a577d720a2e859c0e80a0035e2f017)
#2 0x7fffda6bbd40 in llvm::RuntimeDyldImpl::emitSection(llvm::object::ObjectFile const&, llvm::object::SectionRef const&, bool) (/lib/x86_64-linux-gnu/libLLVM-14.so.1+0x288dd40) (BuildId: 2bc68177e1745130d2e10f07f1644b9def97578b)
#3 0x7fffda6bae69 in llvm::RuntimeDyldImpl::findOrEmitSection(llvm::object::ObjectFile const&, llvm::object::SectionRef const&, bool, std::map<llvm::object::SectionRef, unsigned int, std::less<llvm::object::SectionRef>, std::allocator<std::pair<llvm::object::SectionRef const, unsigned int> > >&) (/lib/x86_64-linux-gnu/libLLVM-14.so.1+0x288ce69) (BuildId: 2bc68177e1745130d2e10f07f1644b9def97578b)
#4 0x7fffda6b9431 in llvm::RuntimeDyldImpl::loadObjectImpl(llvm::object::ObjectFile const&) (/lib/
....
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV asan_interceptors.cpp.o in __asan::QuickCheckForUnpoisonedRegion(unsigned long, unsigned long)
==39275==ABORTING
* When `ARROW_USE_ASAN` and `ARROW_USE_UBSAN` are set to `OFF`
* I saw CI still reported the same error (but there was only one case reported such an error), https://github.com/apache/arrow/actions/runs/7150176834/job/19473189846?pr=39098
* My local env reported no error, and all tests passed
I have no idea how to address this test case failure currently and will spend some more time on investigation. Just in case this is triggered by LLJIT's sanitizing issue, do we somehow suppress third party library's ASAN/UBSAN issues anywhere? Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1428845444
##########
cpp/src/gandiva/engine.cc:
##########
@@ -86,6 +88,13 @@
#include <llvm/Transforms/Utils.h>
#include <llvm/Transforms/Vectorize.h>
+// JITLink is available in LLVM 9+
+// but the `InProcessMemoryManager::Create` API was added since LLVM 14
+#if LLVM_VERSION_MAJOR >= 14 && !defined(_WIN32) && !defined(_WIN64)
+#define JIT_LINK_SUPPORTED 1
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1858856388
> Can we keep this? This was introduced by https://github.com/apache/arrow/pull/6417 .
Keeping this API will require us to expose the `Configuration` class via Python API as well. I will take a look later to see how this can be done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1428845486
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,135 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
+ if (!expected) {
+ return Status::CodeGenError(error_context, llvm::toString(expected.takeError()));
+ }
+ return std::move(expected.get());
+}
+
+Result<llvm::orc::JITTargetMachineBuilder> GetTargetMachineBuilder(
+ const Configuration& conf) {
+ llvm::orc::JITTargetMachineBuilder jtmb(
+ (llvm::Triple(llvm::sys::getDefaultTargetTriple())));
+ if (conf.target_host_cpu()) {
+ jtmb.setCPU(cpu_name.str());
+ jtmb.addFeatures(cpu_attrs);
+ }
+ auto const opt_level =
+ conf.optimize() ? llvm::CodeGenOpt::Aggressive : llvm::CodeGenOpt::None;
+ jtmb.setCodeGenOptLevel(opt_level);
+ return jtmb;
+}
+
+std::string GetModuleIR(const llvm::Module& module) {
+ std::string ir;
+ llvm::raw_string_ostream stream(ir);
+ module.print(stream, nullptr);
+ return ir;
+}
+
+void AddAbsoluteSymbol(llvm::orc::LLJIT& lljit, const std::string& name,
+ void* function_ptr) {
+ llvm::orc::MangleAndInterner mangle(lljit.getExecutionSession(), lljit.getDataLayout());
+
+ // https://github.com/llvm/llvm-project/commit/8b1771bd9f304be39d4dcbdcccedb6d3bcd18200#diff-77984a824d9182e5c67a481740f3bc5da78d5bd4cf6e1716a083ddb30a4a4931
+ // LLVM 17 introduced ExecutorSymbolDef and move most of ORC APIs to ExecutorAddr
+#if LLVM_VERSION_MAJOR >= 17
+ llvm::orc::ExecutorSymbolDef symbol(
+ llvm::orc::ExecutorAddr(reinterpret_cast<uint64_t>(function_ptr)),
+ llvm::JITSymbolFlags::Exported);
+#else
+ llvm::JITEvaluatedSymbol symbol(reinterpret_cast<llvm::JITTargetAddress>(function_ptr),
+ llvm::JITSymbolFlags::Exported);
+#endif
+
+ auto error = lljit.getMainJITDylib().define(
+ llvm::orc::absoluteSymbols({{mangle(name), symbol}}));
+ llvm::cantFail(std::move(error));
+}
+
+// add current process symbol to dylib
+// LLVM >= 18 does this automatically
+void AddProcessSymbol(llvm::orc::LLJIT& lljit) {
+ lljit.getMainJITDylib().addGenerator(
+ llvm::cantFail(llvm::orc::DynamicLibrarySearchGenerator::GetForCurrentProcess(
+ lljit.getDataLayout().getGlobalPrefix())));
+ // the `atexit` symbol cannot be found for ASAN
+#if defined(__SANITIZE_ADDRESS__) || \
+ (defined(__has_feature) && (__has_feature(address_sanitizer)))
+ if (!lljit.lookup("atexit")) {
+ AddAbsoluteSymbol(lljit, "atexit", reinterpret_cast<void*>(atexit));
+ }
+#endif
+}
+
+#if JIT_LINK_SUPPORTED
+Result<std::unique_ptr<llvm::jitlink::InProcessMemoryManager>> CreateMemmoryManager() {
+ auto maybe_mem_manager = llvm::jitlink::InProcessMemoryManager::Create();
+ ARROW_ASSIGN_OR_RAISE(
+ auto memory_manager,
+ AsArrowResult(maybe_mem_manager, "Could not create memory manager: "));
+ return memory_manager;
Review Comment:
Good catch. Updated
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,135 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
+ if (!expected) {
+ return Status::CodeGenError(error_context, llvm::toString(expected.takeError()));
+ }
+ return std::move(expected.get());
+}
+
+Result<llvm::orc::JITTargetMachineBuilder> GetTargetMachineBuilder(
+ const Configuration& conf) {
+ llvm::orc::JITTargetMachineBuilder jtmb(
+ (llvm::Triple(llvm::sys::getDefaultTargetTriple())));
+ if (conf.target_host_cpu()) {
+ jtmb.setCPU(cpu_name.str());
+ jtmb.addFeatures(cpu_attrs);
+ }
+ auto const opt_level =
+ conf.optimize() ? llvm::CodeGenOpt::Aggressive : llvm::CodeGenOpt::None;
+ jtmb.setCodeGenOptLevel(opt_level);
+ return jtmb;
+}
+
+std::string GetModuleIR(const llvm::Module& module) {
+ std::string ir;
+ llvm::raw_string_ostream stream(ir);
+ module.print(stream, nullptr);
+ return ir;
+}
+
+void AddAbsoluteSymbol(llvm::orc::LLJIT& lljit, const std::string& name,
+ void* function_ptr) {
+ llvm::orc::MangleAndInterner mangle(lljit.getExecutionSession(), lljit.getDataLayout());
+
+ // https://github.com/llvm/llvm-project/commit/8b1771bd9f304be39d4dcbdcccedb6d3bcd18200#diff-77984a824d9182e5c67a481740f3bc5da78d5bd4cf6e1716a083ddb30a4a4931
+ // LLVM 17 introduced ExecutorSymbolDef and move most of ORC APIs to ExecutorAddr
+#if LLVM_VERSION_MAJOR >= 17
+ llvm::orc::ExecutorSymbolDef symbol(
+ llvm::orc::ExecutorAddr(reinterpret_cast<uint64_t>(function_ptr)),
+ llvm::JITSymbolFlags::Exported);
+#else
+ llvm::JITEvaluatedSymbol symbol(reinterpret_cast<llvm::JITTargetAddress>(function_ptr),
+ llvm::JITSymbolFlags::Exported);
+#endif
+
+ auto error = lljit.getMainJITDylib().define(
+ llvm::orc::absoluteSymbols({{mangle(name), symbol}}));
+ llvm::cantFail(std::move(error));
+}
+
+// add current process symbol to dylib
+// LLVM >= 18 does this automatically
+void AddProcessSymbol(llvm::orc::LLJIT& lljit) {
+ lljit.getMainJITDylib().addGenerator(
+ llvm::cantFail(llvm::orc::DynamicLibrarySearchGenerator::GetForCurrentProcess(
+ lljit.getDataLayout().getGlobalPrefix())));
+ // the `atexit` symbol cannot be found for ASAN
+#if defined(__SANITIZE_ADDRESS__) || \
+ (defined(__has_feature) && (__has_feature(address_sanitizer)))
+ if (!lljit.lookup("atexit")) {
+ AddAbsoluteSymbol(lljit, "atexit", reinterpret_cast<void*>(atexit));
+ }
+#endif
+}
+
+#if JIT_LINK_SUPPORTED
+Result<std::unique_ptr<llvm::jitlink::InProcessMemoryManager>> CreateMemmoryManager() {
+ auto maybe_mem_manager = llvm::jitlink::InProcessMemoryManager::Create();
+ ARROW_ASSIGN_OR_RAISE(
+ auto memory_manager,
+ AsArrowResult(maybe_mem_manager, "Could not create memory manager: "));
+ return memory_manager;
+}
+
+Status UseJitLinkIfEnabled(llvm::orc::LLJITBuilder& jit_builder) {
+ static auto maybe_use_jit_link = ::arrow::internal::GetEnvVar("GANDIVA_USE_JIT_LINK");
+ if (maybe_use_jit_link.ok()) {
+ ARROW_ASSIGN_OR_RAISE(static auto memory_manager, CreateMemmoryManager());
+ jit_builder.setObjectLinkingLayerCreator(
+ [&](llvm::orc::ExecutionSession& ES, const llvm::Triple& TT) {
+ return std::make_unique<llvm::orc::ObjectLinkingLayer>(ES, *memory_manager);
+ });
+ }
+ return Status::OK();
+}
+#endif
+
+Result<std::unique_ptr<llvm::orc::LLJIT>> BuildJIT(
+ llvm::orc::JITTargetMachineBuilder jtmb,
+ std::optional<std::reference_wrapper<GandivaObjectCache>>& object_cache) {
+ auto jit_builder = llvm::orc::LLJITBuilder();
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1846297152
> I spent some time to try to get it up and running, and found the `amd64/ubuntu:20.04` base image defines such env vars, and this causes all network traffic to go through `localhost:8080` but there is no such proxy running and all the package installation fail because of this.
Oh, really? How did you confirm it?
I couldn't find it:
```console
$ docker run -it --rm amd64/ubuntu:20.04
root@6d3439e4a60d:/# env
HOSTNAME=6d3439e4a60d
PWD=/
HOME=/root
LS_COLORS=rs=0:di=01;34:ln=01;36:mh=00:pi=40;33:so=01;35:do=01;35:bd=40;33;01:cd=40;33;01:or=40;31;01:mi=00:su=37;41:sg=30;43:ca=30;41:tw=30;42:ow=34;42:st=37;44:ex=01;32:*.tar=01;31:*.tgz=01;31:*.arc=01;31:*.arj=01;31:*.taz=01;31:*.lha=01;31:*.lz4=01;31:*.lzh=01;31:*.lzma=01;31:*.tlz=01;31:*.txz=01;31:*.tzo=01;31:*.t7z=01;31:*.zip=01;31:*.z=01;31:*.dz=01;31:*.gz=01;31:*.lrz=01;31:*.lz=01;31:*.lzo=01;31:*.xz=01;31:*.zst=01;31:*.tzst=01;31:*.bz2=01;31:*.bz=01;31:*.tbz=01;31:*.tbz2=01;31:*.tz=01;31:*.deb=01;31:*.rpm=01;31:*.jar=01;31:*.war=01;31:*.ear=01;31:*.sar=01;31:*.rar=01;31:*.alz=01;31:*.ace=01;31:*.zoo=01;31:*.cpio=01;31:*.7z=01;31:*.rz=01;31:*.cab=01;31:*.wim=01;31:*.swm=01;31:*.dwm=01;31:*.esd=01;31:*.jpg=01;35:*.jpeg=01;35:*.mjpg=01;35:*.mjpeg=01;35:*.gif=01;35:*.bmp=01;35:*.pbm=01;35:*.pgm=01;35:*.ppm=01;35:*.tga=01;35:*.xbm=01;35:*.xpm=01;35:*.tif=01;35:*.tiff=01;35:*.png=01;35:*.svg=01;35:*.svgz=01;35:*.mng=01;35:*.pcx=01;35:*.mov=01;35:*.mpg=01;35:*.mpeg=01;35:*.m2v=0
1;35:*.mkv=01;35:*.webm=01;35:*.ogm=01;35:*.mp4=01;35:*.m4v=01;35:*.mp4v=01;35:*.vob=01;35:*.qt=01;35:*.nuv=01;35:*.wmv=01;35:*.asf=01;35:*.rm=01;35:*.rmvb=01;35:*.flc=01;35:*.avi=01;35:*.fli=01;35:*.flv=01;35:*.gl=01;35:*.dl=01;35:*.xcf=01;35:*.xwd=01;35:*.yuv=01;35:*.cgm=01;35:*.emf=01;35:*.ogv=01;35:*.ogx=01;35:*.aac=00;36:*.au=00;36:*.flac=00;36:*.m4a=00;36:*.mid=00;36:*.midi=00;36:*.mka=00;36:*.mp3=00;36:*.mpc=00;36:*.ogg=00;36:*.ra=00;36:*.wav=00;36:*.oga=00;36:*.opus=00;36:*.spx=00;36:*.xspf=00;36:
TERM=xterm
SHLVL=1
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
_=/usr/bin/env
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #39098:
URL: https://github.com/apache/arrow/pull/39098
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1854753925
The remained ASAN error is https://github.com/apache/arrow/pull/39098#issuecomment-1852177839 , right?
I'll try it too.
BTW, how did you generate the benchmark result graph!? It's very easy to understand! I thought always the Google Benchmark text output is hard to understand...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1853836279
@kou thanks so much for the help. Now I added such an `atexit` symbol for `ASAN` only, and it should not affect non `ASAN` related environment. In my macOS environment, `ASAN` still reports the `ASAN` error even with this change, but the CI doesn't report any issue any more. I will use this approach for the time being, and may do some more investigation later to see if this is the best way we could take.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1844974455
> You may reproduce this on your macOS with Docker
Thanks. I will give it a try later. I found that the crash in `C++ / ARM64 Ubuntu 20.04 C++ (pull_request)` is because LLVM 10 returns a symbol for a given function name successfully, however, for some reason, the function address in the symbol is 0, which causes the function pointer to be null. Previously only the returned `Expected` is checked and its address is not checked if successful status. Now I added an additional check for the function address, so it stops crashing, but the test case still fails in this CI env.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1859134570
> Can we keep this? This was introduced by https://github.com/apache/arrow/pull/6417
@kou I added it back by exposing a new `Configuration` class in python binding, could you please check it out? I am not familiar with cython and only follow the existing code to get it to work, and please advise. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1429269659
##########
cpp/src/gandiva/engine.cc:
##########
@@ -454,10 +555,9 @@ arrow::Status Engine::AddGlobalMappings() {
}
std::string Engine::DumpIR() {
Review Comment:
OK. How about adding new `const std::string& Engine::ir()` and keeping `ARROW_DEPRECATED("Deprecated in 15.0.0. Use ir() instead.") std::string Engine:::DumpIR() {return ir();}` for backward compatibility?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1852177839
With my current macOS + Docker + Rosetta setup, I did some more experiments today, and I slightly modified the `ubuntu-22.04-cpp.dockerfile` to create an `ubuntu-23.10-cpp.dockerfile`, and install LLVM 16 and 17 for verification, and I found:
1) LLVM 16 reported similar error as LLVM 14 (under Ubuntu 22.04), with slightly different stack trace (the `#0` stack is now `memcpy`, may be caused by different version of address sanitizer in LLVM)
```
AddressSanitizer:DEADLYSIGNAL
=================================================================
==46227==ERROR: AddressSanitizer: SEGV on unknown address 0x200071e56200 (pc 0x55555597eb3a bp 0x5554ffffd280 sp 0x5554ffffca40 T0)
==46227==The signal is caused by a READ memory access.
#0 0x55555597eb3a in memcpy (/build/cpp/debug/gandiva-internals-test+0x42ab3a) (BuildId: 30f326eec529ed661b538f9254c6b415894cb6d3)
#1 0x7fffdc6b8d39 in llvm::RuntimeDyldImpl::emitSection(llvm::object::ObjectFile const&, llvm::object::SectionRef const&, bool) (/usr/lib/llvm-16/lib/libLLVM-16.so.1+0x2aded39) (BuildId: 08068dfc9a0c80774489feea5c7df0a25abf4ced)
#2 0x7fffdc6b7e02 in llvm::RuntimeDyldImpl::findOrEmitSection(llvm::object::ObjectFile const&, llvm::object::SectionRef const&, bool, std::map<llvm::object::SectionRef, unsigned int, std::less<llvm::object::SectionRef>, std::allocator<std::pair<llvm::object::SectionRef const, unsigned int>>>&) (/usr/lib/llvm-16/lib/libLLVM-16.so.1+0x2adde02) (BuildId: 08068dfc9a0c80774489feea5c7df0a25abf4ced)
#3 0x7fffdc6b6580 in llvm::RuntimeDyldImpl::loadObjectImpl(llvm::object::ObjectFile const&) (/usr/lib/llvm-16/lib/libLLVM-16.so.1+0x2adc580) (BuildId: 08068dfc9a0c80774489feea5c7df0a25abf4ced)
#4 0x7fffdc6c9bb5 in llvm::RuntimeDyldELF::loadObject(llvm::object::ObjectFile const&) (/usr/lib/llvm-16/lib/libLLVM-16.so.1+0x2aefbb5) (BuildId: 08068dfc9a0c80774489feea5c7df0a25abf4ced)
#5 0x7fffdc6baf57 in llvm::RuntimeDyld::loadObject(llvm::object::ObjectFile const&) (/usr/lib/llvm-16/lib/libLLVM-16.so.1+0x2ae0f57) (BuildId: 08068dfc9a0c80774489feea5c7df0a25abf4ced)
```
2) LLVM 17 doesn't report any error. Namely, the same code branch `feature/orc-v2-jit`, when compiled and run in LLVM 17 (Ubuntu 23.10), the ASAN issue is gone, and all tests passed without any failure or error. I verified the binary is indeed compiled with ASAN enabled by running `nm ./debug/gandiva-internals-test | grep asan`
It seems the issue in my local env is probably a LLJIT/LLVM ASAN issue, and I am not sure if it affects the functionality in this case.
But since I am not able to reproduce the exactly same issue in CI, I am not sure the issue in my local env has the same cause of the issue in CI.
@kou, is it possible for your to use LLVM 17 and Ubuntu 23.10 to see how things will turn out, alternatively, if I want to add a new check env `LLVM 17 + Ubuntu 23.10` in CI, which part of configuration should I change? Thanks.
### steps to create the 23.10 docker image for testing
1) copy `ubuntu-22.04-cpp.dockerfile` to `ubuntu-23.10-cpp.dockerfile`
2) change the base image from `22.04` to `23.10`
3) install an additional `libclang-rt-${clang_tools}-dev` package for sanitizer
4) install `libboost-filesystem1.81-dev` and `libboost-system1.81-dev` to use boost 1.81 instead of 1.74
5) disable `install_gcs_testbench.sh` script (its installation failed so I simply remove it during my verification)
6) `LLVM=17 CLANG_TOOLS=17 UBUNTU=23.10 archery docker run ubuntu-cpp-sanitizer bash` to run and verify manually
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1417035796
##########
cpp/src/gandiva/engine.h:
##########
@@ -38,15 +43,20 @@ class GANDIVA_EXPORT Engine {
llvm::LLVMContext* context() { return context_.get(); }
llvm::IRBuilder<>* ir_builder() { return ir_builder_.get(); }
LLVMTypes* types() { return &types_; }
- llvm::Module* module() { return module_; }
+
+ /// Retrieve LLVM module in the engine.
+ /// This should only be called before `FinalizeModule` is called
+ llvm::Module* module();
/// Factory method to create and initialize the engine object.
///
/// \param[in] config the engine configuration
/// \param[in] cached flag to mark if the module is already compiled and cached
- /// \param[out] engine the created engine
- static Status Make(const std::shared_ptr<Configuration>& config, bool cached,
- std::unique_ptr<Engine>* engine);
+ /// \param[in] object_cache an optional object_cache used for building the module
+ /// \return arrow::Result containing the created engine
+ static Result<std::unique_ptr<Engine>> Make(
+ const std::shared_ptr<Configuration>& config, bool cached,
+ std::optional<std::reference_wrapper<GandivaObjectCache>> object_cache = std::nullopt);
Review Comment:
Since this PR has to change the `Engine::Make` and `LLVMGenerator::Make` signatures anyway, I change the factory method to return an `arrow::Result` instead of `arrow::Status` so that it is simpler to use and the optional parameter like `object_cache` can be used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1853865161
Now all test can pass after migrating to ORC v2/LLJIT. I ran the micro benchmark locally, and the performance looks roughly the same.
* I ran 3 times for both locally, and they varied a bit but not differ too much (Google Benchmark itself runs the test a number of iterations)
* the posted data stands for one time result I collected locally
# LLJIT
```
Running release/gandiva-micro-benchmarks
Run on (10 X 24.121 MHz CPU s)
CPU Caches:
L1 Data 64 KiB
L1 Instruction 128 KiB
L2 Unified 4096 KiB (x10)
Load Average: 8.24, 27.86, 29.55
/Users/ss/dev/projects/opensource/arrow/cpp/src/gandiva/cache.cc:50: Creating gandiva cache with capacity of 500
/Users/ss/dev/projects/opensource/arrow/cpp/src/gandiva/engine.cc:211: Detected CPU Name : cyclone
/Users/ss/dev/projects/opensource/arrow/cpp/src/gandiva/engine.cc:212: Detected CPU Features: []
-----------------------------------------------------------------------------------------
Benchmark Time CPU Iterations
-----------------------------------------------------------------------------------------
TimedTestAdd3/min_time:1.000 993 us 992 us 1368
TimedTestBigNested/min_time:1.000 8000 us 7990 us 177
TimedTestExtractYear/min_time:1.000 7267 us 7262 us 198
TimedTestFilterAdd2/min_time:1.000 2829 us 2829 us 486
TimedTestFilterLike/min_time:1.000 12213 us 12212 us 115
TimedTestCastFloatFromString/min_time:1.000 14800 us 14798 us 94
TimedTestCastIntFromString/min_time:1.000 14375 us 14367 us 96
TimedTestAllocs/min_time:1.000 34461 us 34458 us 41
TimedTestOutputStringAllocs/min_time:1.000 52166 us 52158 us 28
TimedTestMultiOr/min_time:1.000 8928 us 8928 us 157
TimedTestInExpr/min_time:1.000 2484 us 2484 us 554
DecimalAdd2Fast/min_time:1.000 1963 us 1962 us 697
DecimalAdd2LeadingZeroes/min_time:1.000 5036 us 5034 us 276
DecimalAdd2LeadingZeroesWithDiv/min_time:1.000 23914 us 23912 us 59
DecimalAdd2Large/min_time:1.000 115643 us 115633 us 12
DecimalAdd3Fast/min_time:1.000 2250 us 2248 us 611
DecimalAdd3LeadingZeroes/min_time:1.000 8654 us 8652 us 162
DecimalAdd3LeadingZeroesWithDiv/min_time:1.000 60001 us 59988 us 23
DecimalAdd3Large/min_time:1.000 235355 us 235343 us 6
```
# MCJIT
```
Running release/gandiva-micro-benchmarks
Run on (10 X 24.116 MHz CPU s)
CPU Caches:
L1 Data 64 KiB
L1 Instruction 128 KiB
L2 Unified 4096 KiB (x10)
Load Average: 5.46, 18.00, 25.05
/Users/ss/dev/projects/opensource/arrow/cpp/src/gandiva/cache.cc:50: Creating gandiva cache with capacity of 500
/Users/ss/dev/projects/opensource/arrow/cpp/src/gandiva/engine.cc:129: Detected CPU Name : cyclone
/Users/ss/dev/projects/opensource/arrow/cpp/src/gandiva/engine.cc:130: Detected CPU Features:
-----------------------------------------------------------------------------------------
Benchmark Time CPU Iterations
-----------------------------------------------------------------------------------------
TimedTestAdd3/min_time:1.000 1147 us 1145 us 1190
TimedTestBigNested/min_time:1.000 7934 us 7923 us 177
TimedTestExtractYear/min_time:1.000 6587 us 6583 us 216
TimedTestFilterAdd2/min_time:1.000 2855 us 2854 us 487
TimedTestFilterLike/min_time:1.000 12447 us 12440 us 114
TimedTestCastFloatFromString/min_time:1.000 14514 us 14495 us 97
TimedTestCastIntFromString/min_time:1.000 13710 us 13694 us 102
TimedTestAllocs/min_time:1.000 33595 us 33568 us 42
TimedTestOutputStringAllocs/min_time:1.000 51769 us 51732 us 27
TimedTestMultiOr/min_time:1.000 12467 us 12462 us 112
TimedTestInExpr/min_time:1.000 2548 us 2547 us 557
DecimalAdd2Fast/min_time:1.000 2024 us 2020 us 678
DecimalAdd2LeadingZeroes/min_time:1.000 5097 us 5091 us 275
DecimalAdd2LeadingZeroesWithDiv/min_time:1.000 23785 us 23760 us 59
DecimalAdd2Large/min_time:1.000 116614 us 116562 us 12
DecimalAdd3Fast/min_time:1.000 2206 us 2205 us 626
DecimalAdd3LeadingZeroes/min_time:1.000 8595 us 8587 us 164
DecimalAdd3LeadingZeroesWithDiv/min_time:1.000 60579 us 60538 us 23
DecimalAdd3Large/min_time:1.000 239852 us 239584 us 6
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1417010832
##########
cpp/src/gandiva/engine.cc:
##########
@@ -163,114 +237,50 @@ Status Engine::LoadFunctionIRs() {
}
/// factory method to construct the engine.
-Status Engine::Make(const std::shared_ptr<Configuration>& conf, bool cached,
- std::unique_ptr<Engine>* out) {
+Result<std::unique_ptr<Engine>> Engine::Make(
+ const std::shared_ptr<Configuration>& conf, bool cached,
+ std::optional<std::reference_wrapper<GandivaObjectCache>> object_cache) {
std::call_once(llvm_init_once_flag, InitOnce);
- auto ctx = std::make_unique<llvm::LLVMContext>();
- auto module = std::make_unique<llvm::Module>("codegen", *ctx);
-
- // Capture before moving, ExecutionEngine does not allow retrieving the
- // original Module.
- auto module_ptr = module.get();
-
- auto opt_level =
- conf->optimize() ? llvm::CodeGenOpt::Aggressive : llvm::CodeGenOpt::None;
-
- // Note that the lifetime of the error string is not captured by the
- // ExecutionEngine but only for the lifetime of the builder. Found by
- // inspecting LLVM sources.
- std::string builder_error;
-
- llvm::EngineBuilder engine_builder(std::move(module));
-
- engine_builder.setEngineKind(llvm::EngineKind::JIT)
- .setOptLevel(opt_level)
- .setErrorStr(&builder_error);
-
- if (conf->target_host_cpu()) {
- engine_builder.setMCPU(cpu_name);
- engine_builder.setMAttrs(cpu_attrs);
- }
- std::unique_ptr<llvm::ExecutionEngine> exec_engine{engine_builder.create()};
-
- if (exec_engine == nullptr) {
- return Status::CodeGenError("Could not instantiate llvm::ExecutionEngine: ",
- builder_error);
- }
Review Comment:
Move the logic into `GetTargetMachineBuilder` and `BuildJIT` functions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1417408376
##########
python/pyarrow/gandiva.pyx:
##########
@@ -186,10 +186,6 @@ cdef class Projector(_Weakrefable):
self.pool = pool
return self
- @property
- def llvm_ir(self):
- return self.projector.get().DumpIR().decode()
Review Comment:
I have to remove the `llvm_ir` API from python binding, since the `Configuration` is not exposed via the `make_projector` and `make_filter` APIs, but we may consider adding it if we can expose the `Configuration` as well
##########
python/pyarrow/gandiva.pyx:
##########
@@ -186,10 +186,6 @@ cdef class Projector(_Weakrefable):
self.pool = pool
return self
- @property
- def llvm_ir(self):
- return self.projector.get().DumpIR().decode()
Review Comment:
I have to remove the `llvm_ir` API from python binding, since the `Configuration` is not exposed via the `make_projector` and `make_filter` APIs, but we may consider adding it back if we can expose the `Configuration` as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1855808716
With my current macOS + Docker + Rosetta setup, I did some more testing today. I verified LLVM 16 + Ubuntu 23.10 + current MCJIT implementation (main branch) + ASAN/UBSAN, and the unit test will report a similar ASAN error like this (but it is for MCJIT instead of LLJIT):
```
AddressSanitizer:DEADLYSIGNAL
=================================================================
==26407==ERROR: AddressSanitizer: SEGV on unknown address 0x20006f564800 (pc 0x55555597cb3a bp 0x5554ffffe0a0 sp 0x5554ffffd860 T0)
==26407==The signal is caused by a READ memory access.
#0 0x55555597cb3a in memcpy (/build/cpp/debug/gandiva-internals-test+0x428b3a) (BuildId: ea73c2cf352befee9264bc2edfc408cc2b2f8a6a)
#1 0x7fffdc795d39 in llvm::RuntimeDyldImpl::emitSection(llvm::object::ObjectFile const&, llvm::object::SectionRef const&, bool) (/usr/lib/llvm-16/lib/libLLVM-16.so.1+0x2aded39) (BuildId: 08068dfc9a0c80774489feea5c7df0a25abf4ced)
#2 0x7fffdc794e02 in llvm::RuntimeDyldImpl::findOrEmitSection(llvm::object::ObjectFile const&, llvm::object::SectionRef const&, bool, std::map<llvm::object::SectionRef, unsigned int, std::less<llvm::object::SectionRef>, std::allocator<std::pair<llvm::object::SectionRef const, unsigned int>>>&) (/usr/lib/llvm-16/lib/libLLVM-16.so.1+0x2adde02) (BuildId: 08068dfc9a0c80774489feea5c7df0a25abf4ced)
#3 0x7fffdc793580 in llvm::RuntimeDyldImpl::loadObjectImpl(llvm::object::ObjectFile const&) (/usr/lib/llvm-16/lib/libLLVM-16.so.1+0x2adc580) (BuildId: 08068dfc9a0c80774489feea5c7df0a25abf4ced)
#4 0x7fffdc7a6bb5 in llvm::RuntimeDyldELF::loadObject(llvm::object::ObjectFile const&) (/usr/lib/llvm-16/lib/libLLVM-16.so.1+0x2aefbb5) (BuildId: 08068dfc9a0c80774489feea5c7df0a25abf4ced)
#5 0x7fffdc797f57 in llvm::RuntimeDyld::loadObject(llvm::object::ObjectFile const&) (/usr/lib/llvm-16/lib/libLLVM-16.so.1+0x2ae0f57) (BuildId: 08068dfc9a0c80774489feea5c7df0a25abf4ced)
#6 0x7fffdc6b00ae in llvm::MCJIT::generateCodeForModule(llvm::Module*) (/usr/lib/llvm-16/lib/libLLVM-16.so.1+0x29f90ae) (BuildId: 08068dfc9a0c80774489feea5c7df0a25abf4ced)
#7 0x7fffdc6b0aec in llvm::MCJIT::finalizeObject() (/usr/lib/llvm-16/lib/libLLVM-16.so.1+0x29f9aec) (BuildId: 08068dfc9a0c80774489feea5c7df0a25abf4ced)
#8 0x7ffffeb2792e in gandiva::Engine::FinalizeModule() /arrow/cpp/src/gandiva/engine.cc:429:22
#9 0x555555a4e27f in gandiva::TestEngine_TestAddUnoptimised_Test::TestBody() /arrow/cpp/src/gandiva/engine_llvm_test.cc:113:3
```
So this issue is likely an ASAN + LLVM issue (for both MCJIT/LLJIT) under this setup (macOS + Docker + Rosetta), and it is not directly related with gandiva's codebase, and it is an issue for LLVM <= 16, and it is not an issue any more since LLVM 17.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1857862643
I did some more experiments using LLVM's new `JITLink` linker [1], which was initially introduced in LLVM 9.0, and can be used together with LLJIT:
* when enabled, it helps to address the ASAN issue on my mac. However, it doesn't address the `atexit` ASAN symbol issue in CI [2]
* I added its support to Gandiva in this PR:
* it is turned off by default since it is less mature compared to `RuntimeDyld` JIT linker, and can be turned on by setting an environment variable
* to keep the change minimum (around 40 lines of code), the current implementation uses some API from LLVM 14, and for lower versions of LLVM, JITLink won't be available
I added a new micro benchmark to verify expression compilation performance. Although claimed to have some advantages [1][3], I don't observe any performance improvement in my limited testing. Neither expression compilation or expression evaluation is faster when `JITLink` is used, and it is roughly the same speed compared to `RuntimeDyld` JIT linker.
# Links
[1] https://llvm.org/docs/JITLink.html
[2] https://github.com/apache/arrow/actions/runs/7221391997/job/19676260548?pr=39098
[3] https://www.phoronix.com/news/LLVM-Lands-JITLink
# Microbenchmark
JITLink enabled micro benchmark, the first micro benchmark `TimedTestExprCompilation` is the newly added benchmark for measuring expression compilation speed.
```
Running ./release/gandiva-micro-benchmarks
Run on (10 X 24.0505 MHz CPU s)
CPU Caches:
L1 Data 64 KiB
L1 Instruction 128 KiB
L2 Unified 4096 KiB (x10)
Load Average: 3.87, 3.56, 5.22
/Users/ss/dev/projects/opensource/arrow/cpp/src/gandiva/cache.cc:50: Creating gandiva cache with capacity of 500
/Users/ss/dev/projects/opensource/arrow/cpp/src/gandiva/engine.cc:247: Detected CPU Name : cyclone
/Users/ss/dev/projects/opensource/arrow/cpp/src/gandiva/engine.cc:248: Detected CPU Features: []
-----------------------------------------------------------------------------------------
Benchmark Time CPU Iterations
-----------------------------------------------------------------------------------------
TimedTestExprCompilation/min_time:1.000 20201 us 20197 us 65
TimedTestAdd3/min_time:1.000 1120 us 1117 us 1247
TimedTestBigNested/min_time:1.000 7872 us 7872 us 177
TimedTestExtractYear/min_time:1.000 7249 us 7242 us 194
TimedTestFilterAdd2/min_time:1.000 2836 us 2836 us 494
TimedTestFilterLike/min_time:1.000 12290 us 12289 us 114
TimedTestCastFloatFromString/min_time:1.000 14320 us 14311 us 99
TimedTestCastIntFromString/min_time:1.000 14288 us 14282 us 99
TimedTestAllocs/min_time:1.000 33997 us 33993 us 41
TimedTestOutputStringAllocs/min_time:1.000 51085 us 51028 us 28
TimedTestMultiOr/min_time:1.000 12630 us 12590 us 113
TimedTestInExpr/min_time:1.000 2515 us 2514 us 557
DecimalAdd2Fast/min_time:1.000 2058 us 2057 us 695
DecimalAdd2LeadingZeroes/min_time:1.000 5158 us 5155 us 272
DecimalAdd2LeadingZeroesWithDiv/min_time:1.000 24319 us 24296 us 58
DecimalAdd2Large/min_time:1.000 115057 us 115046 us 12
DecimalAdd3Fast/min_time:1.000 2290 us 2289 us 612
DecimalAdd3LeadingZeroes/min_time:1.000 8781 us 8780 us 157
DecimalAdd3LeadingZeroesWithDiv/min_time:1.000 61239 us 61232 us 23
DecimalAdd3Large/min_time:1.000 237387 us 237198 us 6
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1417033097
##########
cpp/src/gandiva/engine_llvm_test.cc:
##########
@@ -24,14 +24,14 @@
namespace gandiva {
-typedef int64_t (*add_vector_func_t)(int64_t* data, int n);
+using add_vector_func_t = int64_t (*)(int64_t*, int);
class TestEngine : public ::testing::Test {
protected:
- std::string BuildVecAdd(Engine* engine) {
- auto types = engine->types();
- llvm::IRBuilder<>* builder = engine->ir_builder();
- llvm::LLVMContext* context = engine->context();
+ std::string BuildVecAdd(Engine* gdv_engine) {
Review Comment:
The `engine` param is renamed to `gdv_engine` since it shadows the `engine` member variable which introduces unnecessary confusion here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1842520944
:warning: GitHub issue #37848 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1844855965
Are you working on macOS, right?
You may reproduce this on your macOS with Docker: `LLVM=10 CLANG_TOOLS=10 UBUNTU=20.04 archery docker run ubuntu-cpp`
(I'll try it later on my local Linux machine.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1428845959
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,135 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
+ if (!expected) {
+ return Status::CodeGenError(error_context, llvm::toString(expected.takeError()));
+ }
+ return std::move(expected.get());
+}
+
+Result<llvm::orc::JITTargetMachineBuilder> GetTargetMachineBuilder(
+ const Configuration& conf) {
+ llvm::orc::JITTargetMachineBuilder jtmb(
+ (llvm::Triple(llvm::sys::getDefaultTargetTriple())));
+ if (conf.target_host_cpu()) {
+ jtmb.setCPU(cpu_name.str());
+ jtmb.addFeatures(cpu_attrs);
+ }
+ auto const opt_level =
+ conf.optimize() ? llvm::CodeGenOpt::Aggressive : llvm::CodeGenOpt::None;
+ jtmb.setCodeGenOptLevel(opt_level);
+ return jtmb;
+}
+
+std::string GetModuleIR(const llvm::Module& module) {
Review Comment:
Renamed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1428838772
##########
cpp/src/gandiva/engine.cc:
##########
@@ -86,6 +88,13 @@
#include <llvm/Transforms/Utils.h>
#include <llvm/Transforms/Vectorize.h>
+// JITLink is available in LLVM 9+
+// but the `InProcessMemoryManager::Create` API was added since LLVM 14
+#if LLVM_VERSION_MAJOR >= 14 && !defined(_WIN32) && !defined(_WIN64)
Review Comment:
Hmm. I think that we can skip all Windows with just `!defined(_WIN32)`. Because `_WIN32` is defined on 64bit Windows too.
https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170
> `_WIN32` Defined as 1 when the compilation target is 32-bit ARM, 64-bit ARM, x86, or x64. Otherwise, undefined.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "js8544 (via GitHub)" <gi...@apache.org>.
js8544 commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1434682136
##########
cpp/src/gandiva/engine.h:
##########
@@ -30,23 +35,34 @@
#include "gandiva/llvm_types.h"
#include "gandiva/visibility.h"
+namespace llvm::orc {
+class LLJIT;
+} // namespace llvm::orc
+
namespace gandiva {
/// \brief LLVM Execution engine wrapper.
class GANDIVA_EXPORT Engine {
public:
+ ~Engine();
llvm::LLVMContext* context() { return context_.get(); }
llvm::IRBuilder<>* ir_builder() { return ir_builder_.get(); }
LLVMTypes* types() { return &types_; }
- llvm::Module* module() { return module_; }
+
+ /// Retrieve LLVM module in the engine.
+ /// This should only be called before `FinalizeModule` is called
+ llvm::Module* module();
/// Factory method to create and initialize the engine object.
///
/// \param[in] config the engine configuration
/// \param[in] cached flag to mark if the module is already compiled and cached
- /// \param[out] engine the created engine
- static Status Make(const std::shared_ptr<Configuration>& config, bool cached,
- std::unique_ptr<Engine>* engine);
+ /// \param[in] object_cache an optional object_cache used for building the module
+ /// \return arrow::Result containing the created engine
+ static Result<std::unique_ptr<Engine>> Make(
Review Comment:
Is this a breaking change? We made a lot of Status->Result change before. Typically we would keep both APIs, mark the Status one as deprecated, and remove it in a later release. But it depends on whether Engine is only used internally or not.
##########
cpp/src/gandiva/llvm_generator.h:
##########
@@ -47,15 +49,17 @@ class FunctionHolder;
class GANDIVA_EXPORT LLVMGenerator {
public:
/// \brief Factory method to initialize the generator.
- static Status Make(const std::shared_ptr<Configuration>& config, bool cached,
- std::unique_ptr<LLVMGenerator>* llvm_generator);
+ static Result<std::unique_ptr<LLVMGenerator>> Make(
Review Comment:
Similarly here.
##########
cpp/src/gandiva/engine.cc:
##########
@@ -31,6 +31,7 @@
#include <unordered_set>
#include <utility>
+#include <arrow/util/io_util.h>
Review Comment:
```suggestion
#include "arrow/util/io_util.h"
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1428847833
##########
cpp/src/gandiva/engine_llvm_test.cc:
##########
@@ -111,7 +113,8 @@ TEST_F(TestEngine, TestAddUnoptimised) {
std::string fn_name = BuildVecAdd(engine.get());
ASSERT_OK(engine->FinalizeModule());
- auto add_func = reinterpret_cast<add_vector_func_t>(engine->CompiledFunction(fn_name));
+ EXPECT_OK_AND_ASSIGN(auto fn_ptr, engine->CompiledFunction(fn_name));
Review Comment:
Fixed
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,135 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
+ if (!expected) {
+ return Status::CodeGenError(error_context, llvm::toString(expected.takeError()));
+ }
+ return std::move(expected.get());
+}
+
+Result<llvm::orc::JITTargetMachineBuilder> GetTargetMachineBuilder(
+ const Configuration& conf) {
+ llvm::orc::JITTargetMachineBuilder jtmb(
+ (llvm::Triple(llvm::sys::getDefaultTargetTriple())));
+ if (conf.target_host_cpu()) {
+ jtmb.setCPU(cpu_name.str());
+ jtmb.addFeatures(cpu_attrs);
+ }
+ auto const opt_level =
+ conf.optimize() ? llvm::CodeGenOpt::Aggressive : llvm::CodeGenOpt::None;
+ jtmb.setCodeGenOptLevel(opt_level);
+ return jtmb;
+}
+
+std::string GetModuleIR(const llvm::Module& module) {
+ std::string ir;
+ llvm::raw_string_ostream stream(ir);
+ module.print(stream, nullptr);
+ return ir;
+}
+
+void AddAbsoluteSymbol(llvm::orc::LLJIT& lljit, const std::string& name,
+ void* function_ptr) {
+ llvm::orc::MangleAndInterner mangle(lljit.getExecutionSession(), lljit.getDataLayout());
+
+ // https://github.com/llvm/llvm-project/commit/8b1771bd9f304be39d4dcbdcccedb6d3bcd18200#diff-77984a824d9182e5c67a481740f3bc5da78d5bd4cf6e1716a083ddb30a4a4931
+ // LLVM 17 introduced ExecutorSymbolDef and move most of ORC APIs to ExecutorAddr
+#if LLVM_VERSION_MAJOR >= 17
+ llvm::orc::ExecutorSymbolDef symbol(
+ llvm::orc::ExecutorAddr(reinterpret_cast<uint64_t>(function_ptr)),
+ llvm::JITSymbolFlags::Exported);
+#else
+ llvm::JITEvaluatedSymbol symbol(reinterpret_cast<llvm::JITTargetAddress>(function_ptr),
+ llvm::JITSymbolFlags::Exported);
+#endif
+
+ auto error = lljit.getMainJITDylib().define(
+ llvm::orc::absoluteSymbols({{mangle(name), symbol}}));
+ llvm::cantFail(std::move(error));
+}
+
+// add current process symbol to dylib
+// LLVM >= 18 does this automatically
+void AddProcessSymbol(llvm::orc::LLJIT& lljit) {
+ lljit.getMainJITDylib().addGenerator(
+ llvm::cantFail(llvm::orc::DynamicLibrarySearchGenerator::GetForCurrentProcess(
+ lljit.getDataLayout().getGlobalPrefix())));
+ // the `atexit` symbol cannot be found for ASAN
+#if defined(__SANITIZE_ADDRESS__) || \
+ (defined(__has_feature) && (__has_feature(address_sanitizer)))
+ if (!lljit.lookup("atexit")) {
+ AddAbsoluteSymbol(lljit, "atexit", reinterpret_cast<void*>(atexit));
+ }
+#endif
+}
+
+#if JIT_LINK_SUPPORTED
+Result<std::unique_ptr<llvm::jitlink::InProcessMemoryManager>> CreateMemmoryManager() {
+ auto maybe_mem_manager = llvm::jitlink::InProcessMemoryManager::Create();
+ ARROW_ASSIGN_OR_RAISE(
+ auto memory_manager,
+ AsArrowResult(maybe_mem_manager, "Could not create memory manager: "));
+ return memory_manager;
+}
+
+Status UseJitLinkIfEnabled(llvm::orc::LLJITBuilder& jit_builder) {
Review Comment:
Updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1429270144
##########
cpp/src/gandiva/engine.cc:
##########
@@ -127,28 +262,34 @@ void Engine::InitOnce() {
}
}
ARROW_LOG(INFO) << "Detected CPU Name : " << cpu_name.str();
- ARROW_LOG(INFO) << "Detected CPU Features:" << cpu_attrs_str;
+ ARROW_LOG(INFO) << "Detected CPU Features: [" << cpu_attrs_str << "]";
llvm_init = true;
}
Engine::Engine(const std::shared_ptr<Configuration>& conf,
- std::unique_ptr<llvm::LLVMContext> ctx,
- std::unique_ptr<llvm::ExecutionEngine> engine, llvm::Module* module,
- bool cached)
- : context_(std::move(ctx)),
- execution_engine_(std::move(engine)),
+ std::unique_ptr<llvm::orc::LLJIT> lljit,
+ std::unique_ptr<llvm::TargetMachine> target_machine, bool cached)
+ : context_(std::make_unique<llvm::LLVMContext>()),
+ lljit_(std::move(lljit)),
ir_builder_(std::make_unique<llvm::IRBuilder<>>(*context_)),
- module_(module),
types_(*context_),
optimize_(conf->optimize()),
cached_(cached),
- function_registry_(conf->function_registry()) {}
+ function_registry_(conf->function_registry()),
+ target_machine_(std::move(target_machine)),
+ conf_(conf) {
+ // LLVM 10 doesn't like the expr function name to be the same as the module name
+ auto module_id = "gdv_module_" + std::to_string(reinterpret_cast<int64_t>(this));
+ module_ = std::make_unique<llvm::Module>(module_id, *context_);
+}
+
+Engine::~Engine() {}
Review Comment:
Ah, I see.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1428846258
##########
cpp/src/gandiva/configuration.h:
##########
@@ -65,6 +71,9 @@ class GANDIVA_EXPORT Configuration {
bool target_host_cpu_; /* set the mcpu flag to host cpu while compiling llvm ir */
std::shared_ptr<FunctionRegistry>
function_registry_; /* function registry that may contain external functions */
+ // flag indicating if IR dumping is needed, defaults to false, and turning it on will
+ // negatively affect performance
+ bool needs_ir_dumping_ = false;
Review Comment:
Not problem. I don't like the prefix but I am not sure if `dump_ir` is too simple. Renamed them all to use `dump_ir`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1847464772
> You may reproduce this on your macOS with Docker: LLVM=10 CLANG_TOOLS=10 UBUNTU=20.04 archery docker run ubuntu-cpp
@kou Thanks for the pointer. And I manage to address the issue for LLVM 10 (caused by this [1])
But the `C++ / AMD64 Ubuntu 22.04 C++ ASAN UBSAN` check still fails, with one test case failed. Do you know how I can run this check locally using Docker like above. I assume the container can be built and launched like `LLVM=14 CLANG_TOOLS=14 UBUNTU=22.04 archery docker run ubuntu-cpp`, but I don't know how to turn of the ASAN/UBSAN flow like CI.
BTW, for these two checks:
* `C++ / AMD64 Ubuntu 22.04 C++ ASAN UBSAN`
* `C++ / AMD64 Conda C++ AVX2 `
Do both of them use `LLVM 14 + AMD64 + Ubuntu 22.04`? The same test case does not fail on the latter, do you think if they differ primarily on the ASAN/UBSAN build flags? Thanks.
[1] https://github.com/apache/arrow/pull/39098#discussion_r1420656676
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1848769260
It seems that I could reproduce this with `LLVM=14 CLANG_TOOLS=14 UBUNTU=22.04 archery docker run ubuntu-cpp-sanitizer` on my Debian GNU/Linux machine.
I'll take a look at this later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1420656676
##########
cpp/src/gandiva/llvm_generator_test.cc:
##########
@@ -100,7 +98,9 @@ TEST_F(TestLLVMGenerator, TestAdd) {
auto field_sum = std::make_shared<arrow::Field>("out", arrow::int32());
auto desc_sum = annotator.CheckAndAddInputFieldDescriptor(field_sum);
- std::string fn_name = "codegen";
+ // LLVM 10 doesn't like the expr function name to be the same as the module name when
+ // LLJIT is used
+ std::string fn_name = "llvm_gen_test_add_expr";
Review Comment:
There is a weird behavior of LLVM LLJIT. The LLVM module is created with name `codegen`, if later a function is created with the same name `codegen`, it can be successfully created, when looking up its function pointer, a successful symbol will be returned, however, the function address inside the symbol is 0, making it a nullptr and cannot be used.
* This issue happened for LLVM 10 (not sure if it is Linux specific), and the same code doesn't have such an issue when using LLVM 14.0.6 on macOS
* This issue is specific for ORC v2/LLJIT, and MCJIT doesn't have such an issue
To workaround this issue, I have to:
1) rename the function name in the test case to be more specific and avoid conflict
2) rename the LLVM module name to be more specific and add some random suffix (the object's address) to avoid conflict, and it is very unlikely to encounter such conflict in reality after this change
UPDATE:
I reported a bug to LLVM, https://github.com/llvm/llvm-project/issues/74903
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1852726071
Sorry for not working on this yet...
It seems that this is related to https://github.com/llvm/llvm-project/issues/61289 .
I tried the following ad-hoc change on local, the test is passed:
```diff
diff --git a/cpp/src/gandiva/engine.cc b/cpp/src/gandiva/engine.cc
index a36fb05b97..a3903e812e 100644
--- a/cpp/src/gandiva/engine.cc
+++ b/cpp/src/gandiva/engine.cc
@@ -143,6 +143,11 @@ static void AddProcessSymbol(llvm::orc::LLJIT& lljit) {
lljit.getMainJITDylib().addGenerator(
llvm::cantFail(llvm::orc::DynamicLibrarySearchGenerator::GetForCurrentProcess(
lljit.getDataLayout().getGlobalPrefix())));
+ llvm::orc::SymbolMap symbols;
+ symbols[lljit.mangleAndIntern("atexit")] = llvm::JITEvaluatedSymbol(
+ static_cast<llvm::JITTargetAddress>(reinterpret_cast<uintptr_t>(&atexit)),
+ llvm::JITSymbolFlags::Exported);
+ llvm::cantFail(lljit.getMainJITDylib().define(llvm::orc::absoluteSymbols(symbols)));
}
static Result<std::unique_ptr<llvm::orc::LLJIT>> BuildJIT(
```
I think that this is needed only when:
* ASAN is enabled (Some additional codes for it may use `atexit()` internally?)
* Old LLVM (I hope that this is already fixed (`atexit()` can be used without additional code) in upstream.)
* `libc` is used
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1428845331
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,135 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
+ if (!expected) {
+ return Status::CodeGenError(error_context, llvm::toString(expected.takeError()));
+ }
+ return std::move(expected.get());
+}
+
+Result<llvm::orc::JITTargetMachineBuilder> GetTargetMachineBuilder(
+ const Configuration& conf) {
+ llvm::orc::JITTargetMachineBuilder jtmb(
+ (llvm::Triple(llvm::sys::getDefaultTargetTriple())));
+ if (conf.target_host_cpu()) {
+ jtmb.setCPU(cpu_name.str());
+ jtmb.addFeatures(cpu_attrs);
+ }
+ auto const opt_level =
+ conf.optimize() ? llvm::CodeGenOpt::Aggressive : llvm::CodeGenOpt::None;
+ jtmb.setCodeGenOptLevel(opt_level);
+ return jtmb;
+}
+
+std::string GetModuleIR(const llvm::Module& module) {
+ std::string ir;
+ llvm::raw_string_ostream stream(ir);
+ module.print(stream, nullptr);
+ return ir;
+}
+
+void AddAbsoluteSymbol(llvm::orc::LLJIT& lljit, const std::string& name,
+ void* function_ptr) {
+ llvm::orc::MangleAndInterner mangle(lljit.getExecutionSession(), lljit.getDataLayout());
+
+ // https://github.com/llvm/llvm-project/commit/8b1771bd9f304be39d4dcbdcccedb6d3bcd18200#diff-77984a824d9182e5c67a481740f3bc5da78d5bd4cf6e1716a083ddb30a4a4931
+ // LLVM 17 introduced ExecutorSymbolDef and move most of ORC APIs to ExecutorAddr
+#if LLVM_VERSION_MAJOR >= 17
+ llvm::orc::ExecutorSymbolDef symbol(
+ llvm::orc::ExecutorAddr(reinterpret_cast<uint64_t>(function_ptr)),
+ llvm::JITSymbolFlags::Exported);
+#else
+ llvm::JITEvaluatedSymbol symbol(reinterpret_cast<llvm::JITTargetAddress>(function_ptr),
+ llvm::JITSymbolFlags::Exported);
+#endif
+
+ auto error = lljit.getMainJITDylib().define(
+ llvm::orc::absoluteSymbols({{mangle(name), symbol}}));
+ llvm::cantFail(std::move(error));
+}
+
+// add current process symbol to dylib
+// LLVM >= 18 does this automatically
+void AddProcessSymbol(llvm::orc::LLJIT& lljit) {
+ lljit.getMainJITDylib().addGenerator(
+ llvm::cantFail(llvm::orc::DynamicLibrarySearchGenerator::GetForCurrentProcess(
+ lljit.getDataLayout().getGlobalPrefix())));
+ // the `atexit` symbol cannot be found for ASAN
+#if defined(__SANITIZE_ADDRESS__) || \
+ (defined(__has_feature) && (__has_feature(address_sanitizer)))
Review Comment:
Fixed
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,135 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
+ if (!expected) {
+ return Status::CodeGenError(error_context, llvm::toString(expected.takeError()));
+ }
+ return std::move(expected.get());
+}
+
+Result<llvm::orc::JITTargetMachineBuilder> GetTargetMachineBuilder(
+ const Configuration& conf) {
+ llvm::orc::JITTargetMachineBuilder jtmb(
+ (llvm::Triple(llvm::sys::getDefaultTargetTriple())));
+ if (conf.target_host_cpu()) {
+ jtmb.setCPU(cpu_name.str());
+ jtmb.addFeatures(cpu_attrs);
+ }
+ auto const opt_level =
+ conf.optimize() ? llvm::CodeGenOpt::Aggressive : llvm::CodeGenOpt::None;
+ jtmb.setCodeGenOptLevel(opt_level);
+ return jtmb;
+}
+
+std::string GetModuleIR(const llvm::Module& module) {
+ std::string ir;
+ llvm::raw_string_ostream stream(ir);
+ module.print(stream, nullptr);
+ return ir;
+}
+
+void AddAbsoluteSymbol(llvm::orc::LLJIT& lljit, const std::string& name,
+ void* function_ptr) {
+ llvm::orc::MangleAndInterner mangle(lljit.getExecutionSession(), lljit.getDataLayout());
+
+ // https://github.com/llvm/llvm-project/commit/8b1771bd9f304be39d4dcbdcccedb6d3bcd18200#diff-77984a824d9182e5c67a481740f3bc5da78d5bd4cf6e1716a083ddb30a4a4931
+ // LLVM 17 introduced ExecutorSymbolDef and move most of ORC APIs to ExecutorAddr
+#if LLVM_VERSION_MAJOR >= 17
+ llvm::orc::ExecutorSymbolDef symbol(
+ llvm::orc::ExecutorAddr(reinterpret_cast<uint64_t>(function_ptr)),
+ llvm::JITSymbolFlags::Exported);
+#else
+ llvm::JITEvaluatedSymbol symbol(reinterpret_cast<llvm::JITTargetAddress>(function_ptr),
+ llvm::JITSymbolFlags::Exported);
+#endif
+
+ auto error = lljit.getMainJITDylib().define(
+ llvm::orc::absoluteSymbols({{mangle(name), symbol}}));
+ llvm::cantFail(std::move(error));
+}
+
+// add current process symbol to dylib
+// LLVM >= 18 does this automatically
+void AddProcessSymbol(llvm::orc::LLJIT& lljit) {
+ lljit.getMainJITDylib().addGenerator(
+ llvm::cantFail(llvm::orc::DynamicLibrarySearchGenerator::GetForCurrentProcess(
+ lljit.getDataLayout().getGlobalPrefix())));
+ // the `atexit` symbol cannot be found for ASAN
+#if defined(__SANITIZE_ADDRESS__) || \
+ (defined(__has_feature) && (__has_feature(address_sanitizer)))
+ if (!lljit.lookup("atexit")) {
+ AddAbsoluteSymbol(lljit, "atexit", reinterpret_cast<void*>(atexit));
+ }
+#endif
+}
+
+#if JIT_LINK_SUPPORTED
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1428845256
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,135 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
+ if (!expected) {
+ return Status::CodeGenError(error_context, llvm::toString(expected.takeError()));
+ }
+ return std::move(expected.get());
+}
+
+Result<llvm::orc::JITTargetMachineBuilder> GetTargetMachineBuilder(
+ const Configuration& conf) {
+ llvm::orc::JITTargetMachineBuilder jtmb(
+ (llvm::Triple(llvm::sys::getDefaultTargetTriple())));
+ if (conf.target_host_cpu()) {
+ jtmb.setCPU(cpu_name.str());
+ jtmb.addFeatures(cpu_attrs);
+ }
+ auto const opt_level =
+ conf.optimize() ? llvm::CodeGenOpt::Aggressive : llvm::CodeGenOpt::None;
+ jtmb.setCodeGenOptLevel(opt_level);
+ return jtmb;
+}
+
+std::string GetModuleIR(const llvm::Module& module) {
+ std::string ir;
+ llvm::raw_string_ostream stream(ir);
+ module.print(stream, nullptr);
+ return ir;
+}
+
+void AddAbsoluteSymbol(llvm::orc::LLJIT& lljit, const std::string& name,
+ void* function_ptr) {
+ llvm::orc::MangleAndInterner mangle(lljit.getExecutionSession(), lljit.getDataLayout());
+
+ // https://github.com/llvm/llvm-project/commit/8b1771bd9f304be39d4dcbdcccedb6d3bcd18200#diff-77984a824d9182e5c67a481740f3bc5da78d5bd4cf6e1716a083ddb30a4a4931
+ // LLVM 17 introduced ExecutorSymbolDef and move most of ORC APIs to ExecutorAddr
+#if LLVM_VERSION_MAJOR >= 17
+ llvm::orc::ExecutorSymbolDef symbol(
+ llvm::orc::ExecutorAddr(reinterpret_cast<uint64_t>(function_ptr)),
+ llvm::JITSymbolFlags::Exported);
+#else
+ llvm::JITEvaluatedSymbol symbol(reinterpret_cast<llvm::JITTargetAddress>(function_ptr),
+ llvm::JITSymbolFlags::Exported);
+#endif
+
+ auto error = lljit.getMainJITDylib().define(
+ llvm::orc::absoluteSymbols({{mangle(name), symbol}}));
+ llvm::cantFail(std::move(error));
Review Comment:
Updated to return `Status`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1430188109
##########
cpp/src/gandiva/engine.cc:
##########
@@ -86,6 +88,13 @@
#include <llvm/Transforms/Utils.h>
#include <llvm/Transforms/Vectorize.h>
+// JITLink is available in LLVM 9+
+// but the `InProcessMemoryManager::Create` API was added since LLVM 14
+#if LLVM_VERSION_MAJOR >= 14 && !defined(_WIN32) && !defined(_WIN64)
Review Comment:
Thanks for the info. Now I remove `!defined(_WIN64)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1428848978
##########
cpp/src/gandiva/engine.cc:
##########
@@ -127,28 +262,34 @@ void Engine::InitOnce() {
}
}
ARROW_LOG(INFO) << "Detected CPU Name : " << cpu_name.str();
- ARROW_LOG(INFO) << "Detected CPU Features:" << cpu_attrs_str;
+ ARROW_LOG(INFO) << "Detected CPU Features: [" << cpu_attrs_str << "]";
llvm_init = true;
}
Engine::Engine(const std::shared_ptr<Configuration>& conf,
- std::unique_ptr<llvm::LLVMContext> ctx,
- std::unique_ptr<llvm::ExecutionEngine> engine, llvm::Module* module,
- bool cached)
- : context_(std::move(ctx)),
- execution_engine_(std::move(engine)),
+ std::unique_ptr<llvm::orc::LLJIT> lljit,
+ std::unique_ptr<llvm::TargetMachine> target_machine, bool cached)
+ : context_(std::make_unique<llvm::LLVMContext>()),
+ lljit_(std::move(lljit)),
ir_builder_(std::make_unique<llvm::IRBuilder<>>(*context_)),
- module_(module),
types_(*context_),
optimize_(conf->optimize()),
cached_(cached),
- function_registry_(conf->function_registry()) {}
+ function_registry_(conf->function_registry()),
+ target_machine_(std::move(target_machine)),
+ conf_(conf) {
+ // LLVM 10 doesn't like the expr function name to be the same as the module name
+ auto module_id = "gdv_module_" + std::to_string(reinterpret_cast<int64_t>(this));
+ module_ = std::make_unique<llvm::Module>(module_id, *context_);
+}
+
+Engine::~Engine() {}
Review Comment:
This is because I don't want to expose too much implementation detail, so I forward declare `LLJIT` in the `engine.h` and include `#include <llvm/ExecutionEngine/Orc/LLJIT.h>` in `engine.cc`, and this requires the destructor to be moved into implementation file (`engine.cc`).
```
namespace llvm::orc {
class LLJIT;
} // namespace llvm::orc
```
Please let me know if this is not desirable, in that case, I will add the `LLJIT.h` into `engine.h` directly
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1858827281
I also tried LLVM 16 + ASAN on my Debian GNU/Linux sid environment. And I couldn't reproduce the ASAN error.
Let's ignore it for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "js8544 (via GitHub)" <gi...@apache.org>.
js8544 commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1434718195
##########
cpp/src/gandiva/engine.h:
##########
@@ -30,23 +35,34 @@
#include "gandiva/llvm_types.h"
#include "gandiva/visibility.h"
+namespace llvm::orc {
+class LLJIT;
+} // namespace llvm::orc
+
namespace gandiva {
/// \brief LLVM Execution engine wrapper.
class GANDIVA_EXPORT Engine {
public:
+ ~Engine();
llvm::LLVMContext* context() { return context_.get(); }
llvm::IRBuilder<>* ir_builder() { return ir_builder_.get(); }
LLVMTypes* types() { return &types_; }
- llvm::Module* module() { return module_; }
+
+ /// Retrieve LLVM module in the engine.
+ /// This should only be called before `FinalizeModule` is called
+ llvm::Module* module();
/// Factory method to create and initialize the engine object.
///
/// \param[in] config the engine configuration
/// \param[in] cached flag to mark if the module is already compiled and cached
- /// \param[out] engine the created engine
- static Status Make(const std::shared_ptr<Configuration>& config, bool cached,
- std::unique_ptr<Engine>* engine);
+ /// \param[in] object_cache an optional object_cache used for building the module
+ /// \return arrow::Result containing the created engine
+ static Result<std::unique_ptr<Engine>> Make(
Review Comment:
I see. It makes sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1417006381
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +103,82 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+static arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
+ if (!expected) {
+ return Status::CodeGenError(error_context, llvm::toString(expected.takeError()));
+ }
+ return std::move(expected.get());
+}
+
+static Result<llvm::orc::JITTargetMachineBuilder> GetTargetMachineBuilder(
+ const Configuration& conf) {
+ llvm::orc::JITTargetMachineBuilder jtmb(
+ (llvm::Triple(llvm::sys::getDefaultTargetTriple())));
+ if (conf.target_host_cpu()) {
+ jtmb.setCPU(cpu_name.str());
+ jtmb.addFeatures(cpu_attrs);
+ }
+ auto opt_level =
+ conf.optimize() ? llvm::CodeGenOpt::Aggressive : llvm::CodeGenOpt::None;
+ jtmb.setCodeGenOptLevel(opt_level);
+ return jtmb;
+}
+
+static std::string GetModuleIR(const llvm::Module& module) {
+ std::string ir;
+ llvm::raw_string_ostream stream(ir);
+ module.print(stream, nullptr);
+ return ir;
+}
+
+// add current process symbol to dylib
+// LLVM >= 18 does this automatically
+static void AddProcessSymbol(llvm::orc::LLJIT& lljit) {
+ lljit.getMainJITDylib().addGenerator(
+ cantFail(llvm::orc::DynamicLibrarySearchGenerator::GetForCurrentProcess(
+ lljit.getDataLayout().getGlobalPrefix())));
+}
+
+static Result<std::unique_ptr<llvm::orc::LLJIT>> BuildJIT(
+ llvm::orc::JITTargetMachineBuilder jtmb,
+ std::optional<std::reference_wrapper<GandivaObjectCache>>& object_cache) {
+ auto jit_builder = llvm::orc::LLJITBuilder();
+
+ jit_builder.setJITTargetMachineBuilder(std::move(jtmb));
+ if (object_cache.has_value()) {
+ jit_builder.setCompileFunctionCreator(
Review Comment:
This is the LLJIT API for customizing the object cache.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1843205596
There are still two checks failing, it is likely they failed for the same reason.
```
JIT session error: Symbols not found: [ atexit ]
/arrow/cpp/src/gandiva/tests/binary_test.cc:66: Failure
Value of: status.ok()
Actual: false
Expected: true
Failed to look up function: expr_0_0 error: Failed to materialize symbols: { (main, { $.codegen.__inits.0, expr_0_0, __orc_init_func.codegen }) }
```
The cause is the `Symbols not found: [ atexit ]`, and the `atexit` should be a C function defined in C standard library, but for some reason, this symbol cannot be found in this AMD64 + Ubuntu 22.04 CI env after switching to LLJIT. Currently I have no idea why this happens and it doesn't happen to my local env, and I will have to do some more investigation to see how I can address it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1845250704
@kou about the Docker approach, do you have any idea why the Docker image tries to use HTTP_PROXY/HTTPS_PROXY `localhost:8080`? I spent some time to try to get it up and running, and found the `amd64/ubuntu:20.04` base image defines such env vars, and this causes all network traffic to go through `localhost:8080` but there is no such proxy running and all the package installation fail because of this. Do I need to set up some http proxy to use it? Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1420656676
##########
cpp/src/gandiva/llvm_generator_test.cc:
##########
@@ -100,7 +98,9 @@ TEST_F(TestLLVMGenerator, TestAdd) {
auto field_sum = std::make_shared<arrow::Field>("out", arrow::int32());
auto desc_sum = annotator.CheckAndAddInputFieldDescriptor(field_sum);
- std::string fn_name = "codegen";
+ // LLVM 10 doesn't like the expr function name to be the same as the module name when
+ // LLJIT is used
+ std::string fn_name = "llvm_gen_test_add_expr";
Review Comment:
There is a weird behavior of LLVM LLJIT. The LLVM module is created with name `codegen`, if later a function is created with the same name `codegen`, it can be successfully created, when looking up its function pointer, a successful symbol will be returned, however, the function address inside the symbol is 0, making it a nullptr and cannot be used.
* This issue happened for LLVM 10 (not sure if it is Linux specific), and the same code doesn't have such an issue when using LLVM 14.0.6 on macOS.
* This issue is specific for ORC v2/LLJIT, and MCJIT doesn't have such an issue
I have to:
1) rename the function name in the test case to be more specific and avoid conflict
2) rename the LLVM module name to be more specific and add some random suffix (the object's address) to avoid conflict
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1865672283
@jorisvandenbossche Could you take a look at Cython changes?
@js8544 Do you want to review this before we merge this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1428845724
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,135 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
+ if (!expected) {
+ return Status::CodeGenError(error_context, llvm::toString(expected.takeError()));
+ }
+ return std::move(expected.get());
+}
+
+Result<llvm::orc::JITTargetMachineBuilder> GetTargetMachineBuilder(
+ const Configuration& conf) {
+ llvm::orc::JITTargetMachineBuilder jtmb(
+ (llvm::Triple(llvm::sys::getDefaultTargetTriple())));
+ if (conf.target_host_cpu()) {
+ jtmb.setCPU(cpu_name.str());
+ jtmb.addFeatures(cpu_attrs);
+ }
+ auto const opt_level =
+ conf.optimize() ? llvm::CodeGenOpt::Aggressive : llvm::CodeGenOpt::None;
+ jtmb.setCodeGenOptLevel(opt_level);
+ return jtmb;
+}
+
+std::string GetModuleIR(const llvm::Module& module) {
+ std::string ir;
+ llvm::raw_string_ostream stream(ir);
+ module.print(stream, nullptr);
+ return ir;
+}
+
+void AddAbsoluteSymbol(llvm::orc::LLJIT& lljit, const std::string& name,
+ void* function_ptr) {
+ llvm::orc::MangleAndInterner mangle(lljit.getExecutionSession(), lljit.getDataLayout());
+
+ // https://github.com/llvm/llvm-project/commit/8b1771bd9f304be39d4dcbdcccedb6d3bcd18200#diff-77984a824d9182e5c67a481740f3bc5da78d5bd4cf6e1716a083ddb30a4a4931
+ // LLVM 17 introduced ExecutorSymbolDef and move most of ORC APIs to ExecutorAddr
+#if LLVM_VERSION_MAJOR >= 17
+ llvm::orc::ExecutorSymbolDef symbol(
+ llvm::orc::ExecutorAddr(reinterpret_cast<uint64_t>(function_ptr)),
+ llvm::JITSymbolFlags::Exported);
+#else
+ llvm::JITEvaluatedSymbol symbol(reinterpret_cast<llvm::JITTargetAddress>(function_ptr),
+ llvm::JITSymbolFlags::Exported);
+#endif
+
+ auto error = lljit.getMainJITDylib().define(
+ llvm::orc::absoluteSymbols({{mangle(name), symbol}}));
+ llvm::cantFail(std::move(error));
+}
+
+// add current process symbol to dylib
+// LLVM >= 18 does this automatically
+void AddProcessSymbol(llvm::orc::LLJIT& lljit) {
+ lljit.getMainJITDylib().addGenerator(
+ llvm::cantFail(llvm::orc::DynamicLibrarySearchGenerator::GetForCurrentProcess(
+ lljit.getDataLayout().getGlobalPrefix())));
+ // the `atexit` symbol cannot be found for ASAN
+#if defined(__SANITIZE_ADDRESS__) || \
+ (defined(__has_feature) && (__has_feature(address_sanitizer)))
+ if (!lljit.lookup("atexit")) {
+ AddAbsoluteSymbol(lljit, "atexit", reinterpret_cast<void*>(atexit));
+ }
+#endif
+}
+
+#if JIT_LINK_SUPPORTED
+Result<std::unique_ptr<llvm::jitlink::InProcessMemoryManager>> CreateMemmoryManager() {
+ auto maybe_mem_manager = llvm::jitlink::InProcessMemoryManager::Create();
+ ARROW_ASSIGN_OR_RAISE(
+ auto memory_manager,
+ AsArrowResult(maybe_mem_manager, "Could not create memory manager: "));
+ return memory_manager;
+}
+
+Status UseJitLinkIfEnabled(llvm::orc::LLJITBuilder& jit_builder) {
+ static auto maybe_use_jit_link = ::arrow::internal::GetEnvVar("GANDIVA_USE_JIT_LINK");
+ if (maybe_use_jit_link.ok()) {
+ ARROW_ASSIGN_OR_RAISE(static auto memory_manager, CreateMemmoryManager());
+ jit_builder.setObjectLinkingLayerCreator(
+ [&](llvm::orc::ExecutionSession& ES, const llvm::Triple& TT) {
+ return std::make_unique<llvm::orc::ObjectLinkingLayer>(ES, *memory_manager);
+ });
+ }
+ return Status::OK();
+}
+#endif
+
+Result<std::unique_ptr<llvm::orc::LLJIT>> BuildJIT(
+ llvm::orc::JITTargetMachineBuilder jtmb,
+ std::optional<std::reference_wrapper<GandivaObjectCache>>& object_cache) {
+ auto jit_builder = llvm::orc::LLJITBuilder();
+
+#if JIT_LINK_SUPPORTED
+ ARROW_RETURN_NOT_OK(UseJitLinkIfEnabled(jit_builder));
+#endif
+
+ jit_builder.setJITTargetMachineBuilder(std::move(jtmb));
+ if (object_cache.has_value()) {
+ jit_builder.setCompileFunctionCreator(
+ [&object_cache](llvm::orc::JITTargetMachineBuilder JTMB)
+ -> llvm::Expected<std::unique_ptr<llvm::orc::IRCompileLayer::IRCompiler>> {
+ auto target_machine = JTMB.createTargetMachine();
+ if (!target_machine) {
+ return target_machine.takeError();
+ }
+ // after compilation, the object code will be stored into the given object cache
+ return std::make_unique<llvm::orc::TMOwningSimpleCompiler>(
+ std::move(*target_machine), &object_cache.value().get());
+ });
+ }
+ auto maybe_jit = jit_builder.create();
+ ARROW_ASSIGN_OR_RAISE(auto jit,
+ AsArrowResult(maybe_jit, "Could not create LLJIT instance: "));
+
+ AddProcessSymbol(*jit);
+ return jit;
+}
+
+void Engine::SetLLVMObjectCache(GandivaObjectCache& object_cache) {
+ auto cached_buffer = object_cache.getObject(nullptr);
+ if (cached_buffer) {
+ auto error = lljit_->addObjectFile(std::move(cached_buffer));
+ DCHECK(!error) << "Failed to load object cache: " << llvm::toString(std::move(error));
Review Comment:
Updated
##########
cpp/src/gandiva/engine.cc:
##########
@@ -127,28 +262,34 @@ void Engine::InitOnce() {
}
}
ARROW_LOG(INFO) << "Detected CPU Name : " << cpu_name.str();
- ARROW_LOG(INFO) << "Detected CPU Features:" << cpu_attrs_str;
+ ARROW_LOG(INFO) << "Detected CPU Features: [" << cpu_attrs_str << "]";
llvm_init = true;
}
Engine::Engine(const std::shared_ptr<Configuration>& conf,
- std::unique_ptr<llvm::LLVMContext> ctx,
- std::unique_ptr<llvm::ExecutionEngine> engine, llvm::Module* module,
- bool cached)
- : context_(std::move(ctx)),
- execution_engine_(std::move(engine)),
+ std::unique_ptr<llvm::orc::LLJIT> lljit,
+ std::unique_ptr<llvm::TargetMachine> target_machine, bool cached)
+ : context_(std::make_unique<llvm::LLVMContext>()),
+ lljit_(std::move(lljit)),
ir_builder_(std::make_unique<llvm::IRBuilder<>>(*context_)),
- module_(module),
types_(*context_),
optimize_(conf->optimize()),
cached_(cached),
- function_registry_(conf->function_registry()) {}
+ function_registry_(conf->function_registry()),
+ target_machine_(std::move(target_machine)),
+ conf_(conf) {
+ // LLVM 10 doesn't like the expr function name to be the same as the module name
+ auto module_id = "gdv_module_" + std::to_string(reinterpret_cast<int64_t>(this));
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1428820171
##########
cpp/src/gandiva/engine.cc:
##########
@@ -86,6 +88,13 @@
#include <llvm/Transforms/Utils.h>
#include <llvm/Transforms/Vectorize.h>
+// JITLink is available in LLVM 9+
+// but the `InProcessMemoryManager::Create` API was added since LLVM 14
+#if LLVM_VERSION_MAJOR >= 14 && !defined(_WIN32) && !defined(_WIN64)
Review Comment:
Do we need `!defined(_WIN64)` here?
(Does this work on 64bit Windows?)
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,135 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
+ if (!expected) {
+ return Status::CodeGenError(error_context, llvm::toString(expected.takeError()));
+ }
+ return std::move(expected.get());
+}
+
+Result<llvm::orc::JITTargetMachineBuilder> GetTargetMachineBuilder(
+ const Configuration& conf) {
+ llvm::orc::JITTargetMachineBuilder jtmb(
+ (llvm::Triple(llvm::sys::getDefaultTargetTriple())));
+ if (conf.target_host_cpu()) {
+ jtmb.setCPU(cpu_name.str());
+ jtmb.addFeatures(cpu_attrs);
+ }
+ auto const opt_level =
+ conf.optimize() ? llvm::CodeGenOpt::Aggressive : llvm::CodeGenOpt::None;
+ jtmb.setCodeGenOptLevel(opt_level);
+ return jtmb;
+}
+
+std::string GetModuleIR(const llvm::Module& module) {
+ std::string ir;
+ llvm::raw_string_ostream stream(ir);
+ module.print(stream, nullptr);
+ return ir;
+}
+
+void AddAbsoluteSymbol(llvm::orc::LLJIT& lljit, const std::string& name,
+ void* function_ptr) {
+ llvm::orc::MangleAndInterner mangle(lljit.getExecutionSession(), lljit.getDataLayout());
+
+ // https://github.com/llvm/llvm-project/commit/8b1771bd9f304be39d4dcbdcccedb6d3bcd18200#diff-77984a824d9182e5c67a481740f3bc5da78d5bd4cf6e1716a083ddb30a4a4931
+ // LLVM 17 introduced ExecutorSymbolDef and move most of ORC APIs to ExecutorAddr
+#if LLVM_VERSION_MAJOR >= 17
+ llvm::orc::ExecutorSymbolDef symbol(
+ llvm::orc::ExecutorAddr(reinterpret_cast<uint64_t>(function_ptr)),
+ llvm::JITSymbolFlags::Exported);
+#else
+ llvm::JITEvaluatedSymbol symbol(reinterpret_cast<llvm::JITTargetAddress>(function_ptr),
+ llvm::JITSymbolFlags::Exported);
+#endif
+
+ auto error = lljit.getMainJITDylib().define(
+ llvm::orc::absoluteSymbols({{mangle(name), symbol}}));
+ llvm::cantFail(std::move(error));
+}
+
+// add current process symbol to dylib
+// LLVM >= 18 does this automatically
+void AddProcessSymbol(llvm::orc::LLJIT& lljit) {
+ lljit.getMainJITDylib().addGenerator(
+ llvm::cantFail(llvm::orc::DynamicLibrarySearchGenerator::GetForCurrentProcess(
+ lljit.getDataLayout().getGlobalPrefix())));
+ // the `atexit` symbol cannot be found for ASAN
+#if defined(__SANITIZE_ADDRESS__) || \
+ (defined(__has_feature) && (__has_feature(address_sanitizer)))
+ if (!lljit.lookup("atexit")) {
+ AddAbsoluteSymbol(lljit, "atexit", reinterpret_cast<void*>(atexit));
+ }
+#endif
+}
+
+#if JIT_LINK_SUPPORTED
+Result<std::unique_ptr<llvm::jitlink::InProcessMemoryManager>> CreateMemmoryManager() {
+ auto maybe_mem_manager = llvm::jitlink::InProcessMemoryManager::Create();
+ ARROW_ASSIGN_OR_RAISE(
+ auto memory_manager,
+ AsArrowResult(maybe_mem_manager, "Could not create memory manager: "));
+ return memory_manager;
+}
+
+Status UseJitLinkIfEnabled(llvm::orc::LLJITBuilder& jit_builder) {
Review Comment:
```suggestion
Status UseJITLinkIfEnabled(llvm::orc::LLJITBuilder& jit_builder) {
```
##########
cpp/src/gandiva/engine_llvm_test.cc:
##########
@@ -111,7 +113,8 @@ TEST_F(TestEngine, TestAddUnoptimised) {
std::string fn_name = BuildVecAdd(engine.get());
ASSERT_OK(engine->FinalizeModule());
- auto add_func = reinterpret_cast<add_vector_func_t>(engine->CompiledFunction(fn_name));
+ EXPECT_OK_AND_ASSIGN(auto fn_ptr, engine->CompiledFunction(fn_name));
Review Comment:
I think that `ASSERT_OK_AND_ASSIGN()` is better because the following code doesn't work on error here.
```suggestion
ASSERT_OK_AND_ASSIGN(auto fn_ptr, engine->CompiledFunction(fn_name));
```
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,135 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
+ if (!expected) {
+ return Status::CodeGenError(error_context, llvm::toString(expected.takeError()));
+ }
+ return std::move(expected.get());
+}
+
+Result<llvm::orc::JITTargetMachineBuilder> GetTargetMachineBuilder(
+ const Configuration& conf) {
+ llvm::orc::JITTargetMachineBuilder jtmb(
+ (llvm::Triple(llvm::sys::getDefaultTargetTriple())));
+ if (conf.target_host_cpu()) {
+ jtmb.setCPU(cpu_name.str());
+ jtmb.addFeatures(cpu_attrs);
+ }
+ auto const opt_level =
+ conf.optimize() ? llvm::CodeGenOpt::Aggressive : llvm::CodeGenOpt::None;
+ jtmb.setCodeGenOptLevel(opt_level);
+ return jtmb;
+}
+
+std::string GetModuleIR(const llvm::Module& module) {
+ std::string ir;
+ llvm::raw_string_ostream stream(ir);
+ module.print(stream, nullptr);
+ return ir;
+}
+
+void AddAbsoluteSymbol(llvm::orc::LLJIT& lljit, const std::string& name,
+ void* function_ptr) {
+ llvm::orc::MangleAndInterner mangle(lljit.getExecutionSession(), lljit.getDataLayout());
+
+ // https://github.com/llvm/llvm-project/commit/8b1771bd9f304be39d4dcbdcccedb6d3bcd18200#diff-77984a824d9182e5c67a481740f3bc5da78d5bd4cf6e1716a083ddb30a4a4931
+ // LLVM 17 introduced ExecutorSymbolDef and move most of ORC APIs to ExecutorAddr
+#if LLVM_VERSION_MAJOR >= 17
+ llvm::orc::ExecutorSymbolDef symbol(
+ llvm::orc::ExecutorAddr(reinterpret_cast<uint64_t>(function_ptr)),
+ llvm::JITSymbolFlags::Exported);
+#else
+ llvm::JITEvaluatedSymbol symbol(reinterpret_cast<llvm::JITTargetAddress>(function_ptr),
+ llvm::JITSymbolFlags::Exported);
+#endif
+
+ auto error = lljit.getMainJITDylib().define(
+ llvm::orc::absoluteSymbols({{mangle(name), symbol}}));
+ llvm::cantFail(std::move(error));
+}
+
+// add current process symbol to dylib
+// LLVM >= 18 does this automatically
+void AddProcessSymbol(llvm::orc::LLJIT& lljit) {
+ lljit.getMainJITDylib().addGenerator(
+ llvm::cantFail(llvm::orc::DynamicLibrarySearchGenerator::GetForCurrentProcess(
+ lljit.getDataLayout().getGlobalPrefix())));
+ // the `atexit` symbol cannot be found for ASAN
+#if defined(__SANITIZE_ADDRESS__) || \
+ (defined(__has_feature) && (__has_feature(address_sanitizer)))
Review Comment:
We can use `ADDRESS_SANITIZER`:
```suggestion
#ifdef ADDRESS_SANITIZER
```
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,135 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
+ if (!expected) {
+ return Status::CodeGenError(error_context, llvm::toString(expected.takeError()));
+ }
+ return std::move(expected.get());
+}
+
+Result<llvm::orc::JITTargetMachineBuilder> GetTargetMachineBuilder(
+ const Configuration& conf) {
+ llvm::orc::JITTargetMachineBuilder jtmb(
+ (llvm::Triple(llvm::sys::getDefaultTargetTriple())));
+ if (conf.target_host_cpu()) {
+ jtmb.setCPU(cpu_name.str());
+ jtmb.addFeatures(cpu_attrs);
+ }
+ auto const opt_level =
+ conf.optimize() ? llvm::CodeGenOpt::Aggressive : llvm::CodeGenOpt::None;
+ jtmb.setCodeGenOptLevel(opt_level);
+ return jtmb;
+}
+
+std::string GetModuleIR(const llvm::Module& module) {
+ std::string ir;
+ llvm::raw_string_ostream stream(ir);
+ module.print(stream, nullptr);
+ return ir;
+}
+
+void AddAbsoluteSymbol(llvm::orc::LLJIT& lljit, const std::string& name,
+ void* function_ptr) {
+ llvm::orc::MangleAndInterner mangle(lljit.getExecutionSession(), lljit.getDataLayout());
+
+ // https://github.com/llvm/llvm-project/commit/8b1771bd9f304be39d4dcbdcccedb6d3bcd18200#diff-77984a824d9182e5c67a481740f3bc5da78d5bd4cf6e1716a083ddb30a4a4931
+ // LLVM 17 introduced ExecutorSymbolDef and move most of ORC APIs to ExecutorAddr
+#if LLVM_VERSION_MAJOR >= 17
+ llvm::orc::ExecutorSymbolDef symbol(
+ llvm::orc::ExecutorAddr(reinterpret_cast<uint64_t>(function_ptr)),
+ llvm::JITSymbolFlags::Exported);
+#else
+ llvm::JITEvaluatedSymbol symbol(reinterpret_cast<llvm::JITTargetAddress>(function_ptr),
+ llvm::JITSymbolFlags::Exported);
+#endif
+
+ auto error = lljit.getMainJITDylib().define(
+ llvm::orc::absoluteSymbols({{mangle(name), symbol}}));
+ llvm::cantFail(std::move(error));
+}
+
+// add current process symbol to dylib
+// LLVM >= 18 does this automatically
+void AddProcessSymbol(llvm::orc::LLJIT& lljit) {
+ lljit.getMainJITDylib().addGenerator(
+ llvm::cantFail(llvm::orc::DynamicLibrarySearchGenerator::GetForCurrentProcess(
+ lljit.getDataLayout().getGlobalPrefix())));
+ // the `atexit` symbol cannot be found for ASAN
+#if defined(__SANITIZE_ADDRESS__) || \
+ (defined(__has_feature) && (__has_feature(address_sanitizer)))
+ if (!lljit.lookup("atexit")) {
+ AddAbsoluteSymbol(lljit, "atexit", reinterpret_cast<void*>(atexit));
+ }
+#endif
+}
+
+#if JIT_LINK_SUPPORTED
+Result<std::unique_ptr<llvm::jitlink::InProcessMemoryManager>> CreateMemmoryManager() {
+ auto maybe_mem_manager = llvm::jitlink::InProcessMemoryManager::Create();
+ ARROW_ASSIGN_OR_RAISE(
+ auto memory_manager,
+ AsArrowResult(maybe_mem_manager, "Could not create memory manager: "));
+ return memory_manager;
+}
+
+Status UseJitLinkIfEnabled(llvm::orc::LLJITBuilder& jit_builder) {
+ static auto maybe_use_jit_link = ::arrow::internal::GetEnvVar("GANDIVA_USE_JIT_LINK");
+ if (maybe_use_jit_link.ok()) {
+ ARROW_ASSIGN_OR_RAISE(static auto memory_manager, CreateMemmoryManager());
+ jit_builder.setObjectLinkingLayerCreator(
+ [&](llvm::orc::ExecutionSession& ES, const llvm::Triple& TT) {
+ return std::make_unique<llvm::orc::ObjectLinkingLayer>(ES, *memory_manager);
+ });
+ }
+ return Status::OK();
+}
+#endif
+
+Result<std::unique_ptr<llvm::orc::LLJIT>> BuildJIT(
+ llvm::orc::JITTargetMachineBuilder jtmb,
+ std::optional<std::reference_wrapper<GandivaObjectCache>>& object_cache) {
+ auto jit_builder = llvm::orc::LLJITBuilder();
Review Comment:
```suggestion
llvm::orc::LLJITBuilder jit_builder;
```
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,135 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
+ if (!expected) {
+ return Status::CodeGenError(error_context, llvm::toString(expected.takeError()));
+ }
+ return std::move(expected.get());
+}
+
+Result<llvm::orc::JITTargetMachineBuilder> GetTargetMachineBuilder(
+ const Configuration& conf) {
+ llvm::orc::JITTargetMachineBuilder jtmb(
+ (llvm::Triple(llvm::sys::getDefaultTargetTriple())));
+ if (conf.target_host_cpu()) {
+ jtmb.setCPU(cpu_name.str());
+ jtmb.addFeatures(cpu_attrs);
+ }
+ auto const opt_level =
+ conf.optimize() ? llvm::CodeGenOpt::Aggressive : llvm::CodeGenOpt::None;
+ jtmb.setCodeGenOptLevel(opt_level);
+ return jtmb;
+}
+
+std::string GetModuleIR(const llvm::Module& module) {
+ std::string ir;
+ llvm::raw_string_ostream stream(ir);
+ module.print(stream, nullptr);
+ return ir;
+}
+
+void AddAbsoluteSymbol(llvm::orc::LLJIT& lljit, const std::string& name,
+ void* function_ptr) {
+ llvm::orc::MangleAndInterner mangle(lljit.getExecutionSession(), lljit.getDataLayout());
+
+ // https://github.com/llvm/llvm-project/commit/8b1771bd9f304be39d4dcbdcccedb6d3bcd18200#diff-77984a824d9182e5c67a481740f3bc5da78d5bd4cf6e1716a083ddb30a4a4931
+ // LLVM 17 introduced ExecutorSymbolDef and move most of ORC APIs to ExecutorAddr
+#if LLVM_VERSION_MAJOR >= 17
+ llvm::orc::ExecutorSymbolDef symbol(
+ llvm::orc::ExecutorAddr(reinterpret_cast<uint64_t>(function_ptr)),
+ llvm::JITSymbolFlags::Exported);
+#else
+ llvm::JITEvaluatedSymbol symbol(reinterpret_cast<llvm::JITTargetAddress>(function_ptr),
+ llvm::JITSymbolFlags::Exported);
+#endif
+
+ auto error = lljit.getMainJITDylib().define(
+ llvm::orc::absoluteSymbols({{mangle(name), symbol}}));
+ llvm::cantFail(std::move(error));
Review Comment:
Should we return `arrow::Status` here?
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,135 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
+ if (!expected) {
+ return Status::CodeGenError(error_context, llvm::toString(expected.takeError()));
+ }
+ return std::move(expected.get());
+}
+
+Result<llvm::orc::JITTargetMachineBuilder> GetTargetMachineBuilder(
+ const Configuration& conf) {
+ llvm::orc::JITTargetMachineBuilder jtmb(
+ (llvm::Triple(llvm::sys::getDefaultTargetTriple())));
+ if (conf.target_host_cpu()) {
+ jtmb.setCPU(cpu_name.str());
+ jtmb.addFeatures(cpu_attrs);
+ }
+ auto const opt_level =
+ conf.optimize() ? llvm::CodeGenOpt::Aggressive : llvm::CodeGenOpt::None;
+ jtmb.setCodeGenOptLevel(opt_level);
+ return jtmb;
+}
+
+std::string GetModuleIR(const llvm::Module& module) {
+ std::string ir;
+ llvm::raw_string_ostream stream(ir);
+ module.print(stream, nullptr);
+ return ir;
+}
+
+void AddAbsoluteSymbol(llvm::orc::LLJIT& lljit, const std::string& name,
+ void* function_ptr) {
+ llvm::orc::MangleAndInterner mangle(lljit.getExecutionSession(), lljit.getDataLayout());
+
+ // https://github.com/llvm/llvm-project/commit/8b1771bd9f304be39d4dcbdcccedb6d3bcd18200#diff-77984a824d9182e5c67a481740f3bc5da78d5bd4cf6e1716a083ddb30a4a4931
+ // LLVM 17 introduced ExecutorSymbolDef and move most of ORC APIs to ExecutorAddr
+#if LLVM_VERSION_MAJOR >= 17
+ llvm::orc::ExecutorSymbolDef symbol(
+ llvm::orc::ExecutorAddr(reinterpret_cast<uint64_t>(function_ptr)),
+ llvm::JITSymbolFlags::Exported);
+#else
+ llvm::JITEvaluatedSymbol symbol(reinterpret_cast<llvm::JITTargetAddress>(function_ptr),
+ llvm::JITSymbolFlags::Exported);
+#endif
+
+ auto error = lljit.getMainJITDylib().define(
+ llvm::orc::absoluteSymbols({{mangle(name), symbol}}));
+ llvm::cantFail(std::move(error));
+}
+
+// add current process symbol to dylib
+// LLVM >= 18 does this automatically
+void AddProcessSymbol(llvm::orc::LLJIT& lljit) {
+ lljit.getMainJITDylib().addGenerator(
+ llvm::cantFail(llvm::orc::DynamicLibrarySearchGenerator::GetForCurrentProcess(
+ lljit.getDataLayout().getGlobalPrefix())));
+ // the `atexit` symbol cannot be found for ASAN
+#if defined(__SANITIZE_ADDRESS__) || \
+ (defined(__has_feature) && (__has_feature(address_sanitizer)))
+ if (!lljit.lookup("atexit")) {
+ AddAbsoluteSymbol(lljit, "atexit", reinterpret_cast<void*>(atexit));
+ }
+#endif
+}
+
+#if JIT_LINK_SUPPORTED
Review Comment:
```suggestion
#ifdef JIT_LINK_SUPPORTED
```
##########
cpp/src/gandiva/engine.cc:
##########
@@ -86,6 +88,13 @@
#include <llvm/Transforms/Utils.h>
#include <llvm/Transforms/Vectorize.h>
+// JITLink is available in LLVM 9+
+// but the `InProcessMemoryManager::Create` API was added since LLVM 14
+#if LLVM_VERSION_MAJOR >= 14 && !defined(_WIN32) && !defined(_WIN64)
+#define JIT_LINK_SUPPORTED 1
Review Comment:
We don't need a value.
```suggestion
#define JIT_LINK_SUPPORTED
```
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,135 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
+ if (!expected) {
+ return Status::CodeGenError(error_context, llvm::toString(expected.takeError()));
+ }
+ return std::move(expected.get());
+}
+
+Result<llvm::orc::JITTargetMachineBuilder> GetTargetMachineBuilder(
+ const Configuration& conf) {
+ llvm::orc::JITTargetMachineBuilder jtmb(
+ (llvm::Triple(llvm::sys::getDefaultTargetTriple())));
+ if (conf.target_host_cpu()) {
+ jtmb.setCPU(cpu_name.str());
+ jtmb.addFeatures(cpu_attrs);
+ }
+ auto const opt_level =
+ conf.optimize() ? llvm::CodeGenOpt::Aggressive : llvm::CodeGenOpt::None;
+ jtmb.setCodeGenOptLevel(opt_level);
+ return jtmb;
+}
+
+std::string GetModuleIR(const llvm::Module& module) {
+ std::string ir;
+ llvm::raw_string_ostream stream(ir);
+ module.print(stream, nullptr);
+ return ir;
+}
+
+void AddAbsoluteSymbol(llvm::orc::LLJIT& lljit, const std::string& name,
+ void* function_ptr) {
+ llvm::orc::MangleAndInterner mangle(lljit.getExecutionSession(), lljit.getDataLayout());
+
+ // https://github.com/llvm/llvm-project/commit/8b1771bd9f304be39d4dcbdcccedb6d3bcd18200#diff-77984a824d9182e5c67a481740f3bc5da78d5bd4cf6e1716a083ddb30a4a4931
+ // LLVM 17 introduced ExecutorSymbolDef and move most of ORC APIs to ExecutorAddr
+#if LLVM_VERSION_MAJOR >= 17
+ llvm::orc::ExecutorSymbolDef symbol(
+ llvm::orc::ExecutorAddr(reinterpret_cast<uint64_t>(function_ptr)),
+ llvm::JITSymbolFlags::Exported);
+#else
+ llvm::JITEvaluatedSymbol symbol(reinterpret_cast<llvm::JITTargetAddress>(function_ptr),
+ llvm::JITSymbolFlags::Exported);
+#endif
+
+ auto error = lljit.getMainJITDylib().define(
+ llvm::orc::absoluteSymbols({{mangle(name), symbol}}));
+ llvm::cantFail(std::move(error));
+}
+
+// add current process symbol to dylib
+// LLVM >= 18 does this automatically
+void AddProcessSymbol(llvm::orc::LLJIT& lljit) {
+ lljit.getMainJITDylib().addGenerator(
+ llvm::cantFail(llvm::orc::DynamicLibrarySearchGenerator::GetForCurrentProcess(
+ lljit.getDataLayout().getGlobalPrefix())));
+ // the `atexit` symbol cannot be found for ASAN
+#if defined(__SANITIZE_ADDRESS__) || \
+ (defined(__has_feature) && (__has_feature(address_sanitizer)))
+ if (!lljit.lookup("atexit")) {
+ AddAbsoluteSymbol(lljit, "atexit", reinterpret_cast<void*>(atexit));
+ }
+#endif
+}
+
+#if JIT_LINK_SUPPORTED
+Result<std::unique_ptr<llvm::jitlink::InProcessMemoryManager>> CreateMemmoryManager() {
+ auto maybe_mem_manager = llvm::jitlink::InProcessMemoryManager::Create();
+ ARROW_ASSIGN_OR_RAISE(
+ auto memory_manager,
+ AsArrowResult(maybe_mem_manager, "Could not create memory manager: "));
+ return memory_manager;
+}
+
+Status UseJitLinkIfEnabled(llvm::orc::LLJITBuilder& jit_builder) {
+ static auto maybe_use_jit_link = ::arrow::internal::GetEnvVar("GANDIVA_USE_JIT_LINK");
+ if (maybe_use_jit_link.ok()) {
+ ARROW_ASSIGN_OR_RAISE(static auto memory_manager, CreateMemmoryManager());
+ jit_builder.setObjectLinkingLayerCreator(
+ [&](llvm::orc::ExecutionSession& ES, const llvm::Triple& TT) {
+ return std::make_unique<llvm::orc::ObjectLinkingLayer>(ES, *memory_manager);
+ });
+ }
+ return Status::OK();
+}
+#endif
+
+Result<std::unique_ptr<llvm::orc::LLJIT>> BuildJIT(
+ llvm::orc::JITTargetMachineBuilder jtmb,
+ std::optional<std::reference_wrapper<GandivaObjectCache>>& object_cache) {
+ auto jit_builder = llvm::orc::LLJITBuilder();
+
+#if JIT_LINK_SUPPORTED
+ ARROW_RETURN_NOT_OK(UseJitLinkIfEnabled(jit_builder));
+#endif
+
+ jit_builder.setJITTargetMachineBuilder(std::move(jtmb));
+ if (object_cache.has_value()) {
+ jit_builder.setCompileFunctionCreator(
+ [&object_cache](llvm::orc::JITTargetMachineBuilder JTMB)
+ -> llvm::Expected<std::unique_ptr<llvm::orc::IRCompileLayer::IRCompiler>> {
+ auto target_machine = JTMB.createTargetMachine();
+ if (!target_machine) {
+ return target_machine.takeError();
+ }
+ // after compilation, the object code will be stored into the given object cache
+ return std::make_unique<llvm::orc::TMOwningSimpleCompiler>(
+ std::move(*target_machine), &object_cache.value().get());
+ });
+ }
+ auto maybe_jit = jit_builder.create();
+ ARROW_ASSIGN_OR_RAISE(auto jit,
+ AsArrowResult(maybe_jit, "Could not create LLJIT instance: "));
+
+ AddProcessSymbol(*jit);
+ return jit;
+}
+
+void Engine::SetLLVMObjectCache(GandivaObjectCache& object_cache) {
+ auto cached_buffer = object_cache.getObject(nullptr);
+ if (cached_buffer) {
+ auto error = lljit_->addObjectFile(std::move(cached_buffer));
+ DCHECK(!error) << "Failed to load object cache: " << llvm::toString(std::move(error));
Review Comment:
How about returning `arrow::Status`?
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,135 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
+ if (!expected) {
+ return Status::CodeGenError(error_context, llvm::toString(expected.takeError()));
+ }
+ return std::move(expected.get());
+}
+
+Result<llvm::orc::JITTargetMachineBuilder> GetTargetMachineBuilder(
+ const Configuration& conf) {
+ llvm::orc::JITTargetMachineBuilder jtmb(
+ (llvm::Triple(llvm::sys::getDefaultTargetTriple())));
+ if (conf.target_host_cpu()) {
+ jtmb.setCPU(cpu_name.str());
+ jtmb.addFeatures(cpu_attrs);
+ }
+ auto const opt_level =
+ conf.optimize() ? llvm::CodeGenOpt::Aggressive : llvm::CodeGenOpt::None;
+ jtmb.setCodeGenOptLevel(opt_level);
+ return jtmb;
+}
+
+std::string GetModuleIR(const llvm::Module& module) {
+ std::string ir;
+ llvm::raw_string_ostream stream(ir);
+ module.print(stream, nullptr);
+ return ir;
+}
+
+void AddAbsoluteSymbol(llvm::orc::LLJIT& lljit, const std::string& name,
+ void* function_ptr) {
+ llvm::orc::MangleAndInterner mangle(lljit.getExecutionSession(), lljit.getDataLayout());
+
+ // https://github.com/llvm/llvm-project/commit/8b1771bd9f304be39d4dcbdcccedb6d3bcd18200#diff-77984a824d9182e5c67a481740f3bc5da78d5bd4cf6e1716a083ddb30a4a4931
+ // LLVM 17 introduced ExecutorSymbolDef and move most of ORC APIs to ExecutorAddr
+#if LLVM_VERSION_MAJOR >= 17
+ llvm::orc::ExecutorSymbolDef symbol(
+ llvm::orc::ExecutorAddr(reinterpret_cast<uint64_t>(function_ptr)),
+ llvm::JITSymbolFlags::Exported);
+#else
+ llvm::JITEvaluatedSymbol symbol(reinterpret_cast<llvm::JITTargetAddress>(function_ptr),
+ llvm::JITSymbolFlags::Exported);
+#endif
+
+ auto error = lljit.getMainJITDylib().define(
+ llvm::orc::absoluteSymbols({{mangle(name), symbol}}));
+ llvm::cantFail(std::move(error));
+}
+
+// add current process symbol to dylib
+// LLVM >= 18 does this automatically
+void AddProcessSymbol(llvm::orc::LLJIT& lljit) {
+ lljit.getMainJITDylib().addGenerator(
+ llvm::cantFail(llvm::orc::DynamicLibrarySearchGenerator::GetForCurrentProcess(
+ lljit.getDataLayout().getGlobalPrefix())));
+ // the `atexit` symbol cannot be found for ASAN
+#if defined(__SANITIZE_ADDRESS__) || \
+ (defined(__has_feature) && (__has_feature(address_sanitizer)))
+ if (!lljit.lookup("atexit")) {
+ AddAbsoluteSymbol(lljit, "atexit", reinterpret_cast<void*>(atexit));
+ }
+#endif
+}
+
+#if JIT_LINK_SUPPORTED
+Result<std::unique_ptr<llvm::jitlink::InProcessMemoryManager>> CreateMemmoryManager() {
+ auto maybe_mem_manager = llvm::jitlink::InProcessMemoryManager::Create();
+ ARROW_ASSIGN_OR_RAISE(
+ auto memory_manager,
+ AsArrowResult(maybe_mem_manager, "Could not create memory manager: "));
+ return memory_manager;
Review Comment:
```suggestion
return AsArrowResult(maybe_mem_manager, "Could not create memory manager: ");
```
##########
cpp/src/gandiva/engine.cc:
##########
@@ -127,28 +262,34 @@ void Engine::InitOnce() {
}
}
ARROW_LOG(INFO) << "Detected CPU Name : " << cpu_name.str();
- ARROW_LOG(INFO) << "Detected CPU Features:" << cpu_attrs_str;
+ ARROW_LOG(INFO) << "Detected CPU Features: [" << cpu_attrs_str << "]";
llvm_init = true;
}
Engine::Engine(const std::shared_ptr<Configuration>& conf,
- std::unique_ptr<llvm::LLVMContext> ctx,
- std::unique_ptr<llvm::ExecutionEngine> engine, llvm::Module* module,
- bool cached)
- : context_(std::move(ctx)),
- execution_engine_(std::move(engine)),
+ std::unique_ptr<llvm::orc::LLJIT> lljit,
+ std::unique_ptr<llvm::TargetMachine> target_machine, bool cached)
+ : context_(std::make_unique<llvm::LLVMContext>()),
+ lljit_(std::move(lljit)),
ir_builder_(std::make_unique<llvm::IRBuilder<>>(*context_)),
- module_(module),
types_(*context_),
optimize_(conf->optimize()),
cached_(cached),
- function_registry_(conf->function_registry()) {}
+ function_registry_(conf->function_registry()),
+ target_machine_(std::move(target_machine)),
+ conf_(conf) {
+ // LLVM 10 doesn't like the expr function name to be the same as the module name
+ auto module_id = "gdv_module_" + std::to_string(reinterpret_cast<int64_t>(this));
Review Comment:
```suggestion
auto module_id = "gdv_module_" + std::to_string(reinterpret_cast<uintptr_t>(this));
```
##########
cpp/src/gandiva/llvm_generator_test.cc:
##########
@@ -110,8 +110,10 @@ TEST_F(TestLLVMGenerator, TestAdd) {
auto ir = generator->engine_->DumpIR();
EXPECT_THAT(ir, testing::HasSubstr("vector.body"));
- EvalFunc eval_func = (EvalFunc)generator->engine_->CompiledFunction(fn_name);
+ EXPECT_OK_AND_ASSIGN(auto fn_ptr, generator->engine_->CompiledFunction(fn_name));
+ EXPECT_NE(fn_ptr, nullptr);
Review Comment:
Should we use `ASSERT_*`?
##########
cpp/src/gandiva/engine.cc:
##########
@@ -127,28 +262,34 @@ void Engine::InitOnce() {
}
}
ARROW_LOG(INFO) << "Detected CPU Name : " << cpu_name.str();
- ARROW_LOG(INFO) << "Detected CPU Features:" << cpu_attrs_str;
+ ARROW_LOG(INFO) << "Detected CPU Features: [" << cpu_attrs_str << "]";
llvm_init = true;
}
Engine::Engine(const std::shared_ptr<Configuration>& conf,
- std::unique_ptr<llvm::LLVMContext> ctx,
- std::unique_ptr<llvm::ExecutionEngine> engine, llvm::Module* module,
- bool cached)
- : context_(std::move(ctx)),
- execution_engine_(std::move(engine)),
+ std::unique_ptr<llvm::orc::LLJIT> lljit,
+ std::unique_ptr<llvm::TargetMachine> target_machine, bool cached)
+ : context_(std::make_unique<llvm::LLVMContext>()),
+ lljit_(std::move(lljit)),
ir_builder_(std::make_unique<llvm::IRBuilder<>>(*context_)),
- module_(module),
types_(*context_),
optimize_(conf->optimize()),
cached_(cached),
- function_registry_(conf->function_registry()) {}
+ function_registry_(conf->function_registry()),
+ target_machine_(std::move(target_machine)),
+ conf_(conf) {
+ // LLVM 10 doesn't like the expr function name to be the same as the module name
+ auto module_id = "gdv_module_" + std::to_string(reinterpret_cast<int64_t>(this));
+ module_ = std::make_unique<llvm::Module>(module_id, *context_);
+}
+
+Engine::~Engine() {}
Review Comment:
Do we need this?
##########
cpp/src/gandiva/engine.cc:
##########
@@ -454,10 +555,9 @@ arrow::Status Engine::AddGlobalMappings() {
}
std::string Engine::DumpIR() {
Review Comment:
```suggestion
const std::string& Engine::ir() {
```
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,135 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
+ if (!expected) {
+ return Status::CodeGenError(error_context, llvm::toString(expected.takeError()));
+ }
+ return std::move(expected.get());
+}
+
+Result<llvm::orc::JITTargetMachineBuilder> GetTargetMachineBuilder(
Review Comment:
How about renaming this to `MakeTargetMachineBuilder()` or something because this is not a simple getter function?
##########
cpp/src/gandiva/configuration.h:
##########
@@ -65,6 +71,9 @@ class GANDIVA_EXPORT Configuration {
bool target_host_cpu_; /* set the mcpu flag to host cpu while compiling llvm ir */
std::shared_ptr<FunctionRegistry>
function_registry_; /* function registry that may contain external functions */
+ // flag indicating if IR dumping is needed, defaults to false, and turning it on will
+ // negatively affect performance
+ bool needs_ir_dumping_ = false;
Review Comment:
Do we need `needs_` prefix? It seems that we can just use `dump_ir` or something.
##########
python/pyarrow/gandiva.pyx:
##########
@@ -186,10 +186,6 @@ cdef class Projector(_Weakrefable):
self.pool = pool
return self
- @property
- def llvm_ir(self):
- return self.projector.get().DumpIR().decode()
Review Comment:
Can we keep this? This was introduced by https://github.com/apache/arrow/pull/6417 .
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,135 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
+ if (!expected) {
+ return Status::CodeGenError(error_context, llvm::toString(expected.takeError()));
+ }
+ return std::move(expected.get());
+}
+
+Result<llvm::orc::JITTargetMachineBuilder> GetTargetMachineBuilder(
+ const Configuration& conf) {
+ llvm::orc::JITTargetMachineBuilder jtmb(
+ (llvm::Triple(llvm::sys::getDefaultTargetTriple())));
+ if (conf.target_host_cpu()) {
+ jtmb.setCPU(cpu_name.str());
+ jtmb.addFeatures(cpu_attrs);
+ }
+ auto const opt_level =
+ conf.optimize() ? llvm::CodeGenOpt::Aggressive : llvm::CodeGenOpt::None;
+ jtmb.setCodeGenOptLevel(opt_level);
+ return jtmb;
+}
+
+std::string GetModuleIR(const llvm::Module& module) {
Review Comment:
How about renaming this to `DumpModuleIR()` or something because this is not a simple getter function?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1434712030
##########
cpp/src/gandiva/llvm_generator.h:
##########
@@ -47,15 +49,17 @@ class FunctionHolder;
class GANDIVA_EXPORT LLVMGenerator {
public:
/// \brief Factory method to initialize the generator.
- static Status Make(const std::shared_ptr<Configuration>& config, bool cached,
- std::unique_ptr<LLVMGenerator>* llvm_generator);
+ static Result<std::unique_ptr<LLVMGenerator>> Make(
Review Comment:
Explained above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1434715134
##########
cpp/src/gandiva/engine.cc:
##########
@@ -31,6 +31,7 @@
#include <unordered_set>
#include <utility>
+#include <arrow/util/io_util.h>
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1433912061
##########
cpp/src/gandiva/engine.cc:
##########
@@ -103,9 +112,136 @@ extern const size_t kPrecompiledBitcodeSize;
std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
-static llvm::SmallVector<std::string, 10> cpu_attrs;
+static std::vector<std::string> cpu_attrs;
std::once_flag register_exported_funcs_flag;
+template <typename T>
+arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
+ const std::string& error_context = "") {
Review Comment:
Sure. Updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1877946795
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 83cba25017a5c3a03e47f1851f242fa284f93533.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/20181274047) has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1854951944
> The remained ASAN error is https://github.com/apache/arrow/pull/39098#issuecomment-1852177839 , right?
Correct. It seems to me an ASAN issue for certain LLVM versions since it goes away if switching to LLVM 17, however, I've no idea why this doesn't happen in CI but happened in my local setup.
> how did you generate the benchmark result graph
Nowadays, it is much simpler, I throw the raw text of both Google Benchmarks output to ChatGPT (Plus actually), and prompt it to draw the diagram, and my prompt is:
* copy paste the benchmark outputs
* draw a column chart for benchmark comparison
* represent different series in light coral and light blue colors
* please label the y axis value directly on the chart
* the chart title is "LLJIT vs. MCJIT"
* make x-axis labels tilted for easier reading
* position the y-axis value labels to avoid overlapping, making it easier to read
ChatGPT will parse the benchmark values out of the raw text, and hard code them in a Python script and output the chart. And I tried this a few times previously for such comparison chart and it is good enough and time saving to be useful
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1417015619
##########
cpp/src/gandiva/engine.cc:
##########
@@ -163,114 +237,50 @@ Status Engine::LoadFunctionIRs() {
}
/// factory method to construct the engine.
-Status Engine::Make(const std::shared_ptr<Configuration>& conf, bool cached,
- std::unique_ptr<Engine>* out) {
+Result<std::unique_ptr<Engine>> Engine::Make(
+ const std::shared_ptr<Configuration>& conf, bool cached,
+ std::optional<std::reference_wrapper<GandivaObjectCache>> object_cache) {
std::call_once(llvm_init_once_flag, InitOnce);
- auto ctx = std::make_unique<llvm::LLVMContext>();
- auto module = std::make_unique<llvm::Module>("codegen", *ctx);
-
- // Capture before moving, ExecutionEngine does not allow retrieving the
- // original Module.
- auto module_ptr = module.get();
-
- auto opt_level =
- conf->optimize() ? llvm::CodeGenOpt::Aggressive : llvm::CodeGenOpt::None;
-
- // Note that the lifetime of the error string is not captured by the
- // ExecutionEngine but only for the lifetime of the builder. Found by
- // inspecting LLVM sources.
- std::string builder_error;
-
- llvm::EngineBuilder engine_builder(std::move(module));
-
- engine_builder.setEngineKind(llvm::EngineKind::JIT)
- .setOptLevel(opt_level)
- .setErrorStr(&builder_error);
-
- if (conf->target_host_cpu()) {
- engine_builder.setMCPU(cpu_name);
- engine_builder.setMAttrs(cpu_attrs);
- }
- std::unique_ptr<llvm::ExecutionEngine> exec_engine{engine_builder.create()};
-
- if (exec_engine == nullptr) {
- return Status::CodeGenError("Could not instantiate llvm::ExecutionEngine: ",
- builder_error);
- }
+ ARROW_ASSIGN_OR_RAISE(auto jtmb, GetTargetMachineBuilder(*conf));
+ ARROW_ASSIGN_OR_RAISE(auto jit, BuildJIT(jtmb, object_cache));
+ auto maybe_tm = jtmb.createTargetMachine();
+ ARROW_ASSIGN_OR_RAISE(auto target_machine,
+ AsArrowResult(maybe_tm, "Could not create target machine: "));
std::unique_ptr<Engine> engine{
- new Engine(conf, std::move(ctx), std::move(exec_engine), module_ptr, cached)};
- ARROW_RETURN_NOT_OK(engine->Init());
- *out = std::move(engine);
- return Status::OK();
-}
-
-// This method was modified from its original version for a part of MLIR
-// Original source from
-// https://github.com/llvm/llvm-project/blob/9f2ce5b915a505a5488a5cf91bb0a8efa9ddfff7/mlir/lib/ExecutionEngine/ExecutionEngine.cpp
-// The original copyright notice follows.
-
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-
-static void SetDataLayout(llvm::Module* module) {
- auto target_triple = llvm::sys::getDefaultTargetTriple();
- std::string error_message;
- auto target = llvm::TargetRegistry::lookupTarget(target_triple, error_message);
- if (!target) {
- return;
- }
-
- std::string cpu(llvm::sys::getHostCPUName());
- llvm::SubtargetFeatures features;
- llvm::StringMap<bool> host_features;
-
- if (llvm::sys::getHostCPUFeatures(host_features)) {
- for (auto& f : host_features) {
- features.AddFeature(f.first(), f.second);
- }
- }
-
- std::unique_ptr<llvm::TargetMachine> machine(
- target->createTargetMachine(target_triple, cpu, features.getString(), {}, {}));
-
- module->setDataLayout(machine->createDataLayout());
Review Comment:
Part of the logic here is moved into `GetTargetMachineBuilder` function.
The `setDataLayout` is called in line 265 within `VerifyAndLinkModule` function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1843994901
@kou @wjones127
For one of the failed CI check [1], it reported:
> 87/93 Test #90: gandiva-internals-test .......................***Failed 0.29 sec
> Running gandiva-internals-test, redirecting output into /build/cpp/build/test-logs/gandiva-internals-test.txt (attempt 1/1)
> /arrow/cpp/build-support/run-test.sh: line 88: 27381 Segmentation fault (core dumped) $TEST_EXECUTABLE "$@" > $LOGFILE.raw 2>&1
Do you have any idea how I can view the log file and the core dump file stack? Thanks.
[1] https://github.com/apache/arrow/actions/runs/7115811560/job/19372808518?pr=39098
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1417035796
##########
cpp/src/gandiva/engine.h:
##########
@@ -38,15 +43,20 @@ class GANDIVA_EXPORT Engine {
llvm::LLVMContext* context() { return context_.get(); }
llvm::IRBuilder<>* ir_builder() { return ir_builder_.get(); }
LLVMTypes* types() { return &types_; }
- llvm::Module* module() { return module_; }
+
+ /// Retrieve LLVM module in the engine.
+ /// This should only be called before `FinalizeModule` is called
+ llvm::Module* module();
/// Factory method to create and initialize the engine object.
///
/// \param[in] config the engine configuration
/// \param[in] cached flag to mark if the module is already compiled and cached
- /// \param[out] engine the created engine
- static Status Make(const std::shared_ptr<Configuration>& config, bool cached,
- std::unique_ptr<Engine>* engine);
+ /// \param[in] object_cache an optional object_cache used for building the module
+ /// \return arrow::Result containing the created engine
+ static Result<std::unique_ptr<Engine>> Make(
+ const std::shared_ptr<Configuration>& config, bool cached,
+ std::optional<std::reference_wrapper<GandivaObjectCache>> object_cache = std::nullopt);
Review Comment:
Since this PR have to change the `Engine::Make` and `LLVMGenerator::Make` signatures anyway, I change the factory method to return an `arrow::Result` instead of `arrow::Status` so that it is simpler to use and the optional parameter like `object_cache` can be used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on PR #39098:
URL: https://github.com/apache/arrow/pull/39098#issuecomment-1846869141
> Oh, really? How did you confirm it?
Please ignore my comment above about the proxy stuff. It is likely I set up such proxy long time ago system wide for Docker but forgot it completely after a long time 🤦
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1420656676
##########
cpp/src/gandiva/llvm_generator_test.cc:
##########
@@ -100,7 +98,9 @@ TEST_F(TestLLVMGenerator, TestAdd) {
auto field_sum = std::make_shared<arrow::Field>("out", arrow::int32());
auto desc_sum = annotator.CheckAndAddInputFieldDescriptor(field_sum);
- std::string fn_name = "codegen";
+ // LLVM 10 doesn't like the expr function name to be the same as the module name when
+ // LLJIT is used
+ std::string fn_name = "llvm_gen_test_add_expr";
Review Comment:
There is a weird behavior of LLVM LLJIT. The LLVM module is created with name `codegen`, if later a function is created with the same name `codegen`, it can be successfully created, when looking up its function pointer, a successful symbol will be returned, however, the function address inside the symbol is 0, making it a nullptr and cannot be used.
* This issue happened for LLVM 10 (not sure if it is Linux specific), and the same code doesn't have such an issue when using LLVM 14.0.6 on macOS
* This issue is specific for ORC v2/LLJIT, and MCJIT doesn't have such an issue
To workaround this issue, I have to:
1) rename the function name in the test case to be more specific and avoid conflict
2) rename the LLVM module name to be more specific and add some random suffix (the object's address) to avoid conflict, and it is very unlikely to encounter such a conflict in reality after this change
UPDATE:
I reported a bug to LLVM, https://github.com/llvm/llvm-project/issues/74903
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1420656676
##########
cpp/src/gandiva/llvm_generator_test.cc:
##########
@@ -100,7 +98,9 @@ TEST_F(TestLLVMGenerator, TestAdd) {
auto field_sum = std::make_shared<arrow::Field>("out", arrow::int32());
auto desc_sum = annotator.CheckAndAddInputFieldDescriptor(field_sum);
- std::string fn_name = "codegen";
+ // LLVM 10 doesn't like the expr function name to be the same as the module name when
+ // LLJIT is used
+ std::string fn_name = "llvm_gen_test_add_expr";
Review Comment:
There is a weird behavior of LLVM LLJIT. The LLVM module is created with name `codegen`, if later a function is created with the same name `codegen`, it can be successfully created, when looking up its function pointer, a successful symbol will be returned, however, the function address inside the symbol is 0, making it a nullptr and cannot be used.
* This issue happened for LLVM 10 (not sure if it is Linux specific), and the same code doesn't have such an issue when using LLVM 14.0.6 on macOS
* This issue is specific for ORC v2/LLJIT, and MCJIT doesn't have such an issue
I have to:
1) rename the function name in the test case to be more specific and avoid conflict
2) rename the LLVM module name to be more specific and add some random suffix (the object's address) to avoid conflict
UPDATE:
I reported a bug to LLVM, https://github.com/llvm/llvm-project/issues/74903
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1428837720
##########
cpp/src/gandiva/engine.cc:
##########
@@ -86,6 +88,13 @@
#include <llvm/Transforms/Utils.h>
#include <llvm/Transforms/Vectorize.h>
+// JITLink is available in LLVM 9+
+// but the `InProcessMemoryManager::Create` API was added since LLVM 14
+#if LLVM_VERSION_MAJOR >= 14 && !defined(_WIN32) && !defined(_WIN64)
Review Comment:
I initially didn't include this, but without including `!defined(_WIN64)` makes several CI checks on Windows failed, sorry I didn't kept the specific CI job links for that but I remembered the CI reported something like "incorrect object file format".
`JITLink` does support Windows/COFF format [1][2], but it is fairly new feature, probably landed in LLVM 15 (released in 2022-09), and since I tried to make the `JITLink` related change minimum, and I don't have Windows env to cover different LLVM versions' impact, so I simply skip Windows in this case to make the version check simple.
[1] NATIVE WINDOWS JITING IN LLVM, https://llvm.org/devmtg/2022-11/slides/Tutorial2-JITLink.pdf
[2] 2022 LLVM Dev Mtg: JITLink: Native Windows JITing in LLVM, https://www.youtube.com/watch?v=UwHgCqQ2DDA
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1428846911
##########
cpp/src/gandiva/engine.cc:
##########
@@ -454,10 +555,9 @@ arrow::Status Engine::AddGlobalMappings() {
}
std::string Engine::DumpIR() {
Review Comment:
Updated. But I still keep the API in `Filter` and `Projector` as `DumpIR` since they may be used by apps already (but change them to return `const string&`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org