GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT #39098

niyue · 2023-12-06T09:38:10Z

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.
This PR adds a new JIT linker option JITLink (https://llvm.org/docs/JITLink.html), which can be used together with LLJIT, for LLVM 14+ on Linux/macOS platform. It is turned off by default but could be turned on with environment variable GANDIVA_USE_JIT_LINK

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 dump_ir. If users would like to call DumpIR API of Projector and Filter, they have to set the dump_ir option first.
Closes: [C++][Gandiva] Migration JIT engine from MCJIT to LLJIT #37848

github-actions · 2023-12-06T09:38:40Z

⚠️ GitHub issue #37848 has been automatically assigned in GitHub to PR creator.

niyue · 2023-12-06T09:40:52Z

cpp/src/gandiva/engine.cc

+
+  jit_builder.setJITTargetMachineBuilder(std::move(jtmb));
+  if (object_cache.has_value()) {
+    jit_builder.setCompileFunctionCreator(


This is the LLJIT API for customizing the object cache.

niyue · 2023-12-06T09:44:31Z

cpp/src/gandiva/engine.cc

-  if (exec_engine == nullptr) {
-    return Status::CodeGenError("Could not instantiate llvm::ExecutionEngine: ",
-                                builder_error);
-  }


Move the logic into GetTargetMachineBuilder and BuildJIT functions.

niyue · 2023-12-06T09:47:55Z

cpp/src/gandiva/engine.cc

-  std::unique_ptr<llvm::TargetMachine> machine(
-      target->createTargetMachine(target_triple, cpu, features.getString(), {}, {}));
-
-  module->setDataLayout(machine->createDataLayout());


Part of the logic here is moved into GetTargetMachineBuilder function.

The setDataLayout is called in line 265 within VerifyAndLinkModule function.

niyue · 2023-12-06T10:00:37Z

cpp/src/gandiva/engine_llvm_test.cc

-    auto types = engine->types();
-    llvm::IRBuilder<>* builder = engine->ir_builder();
-    llvm::LLVMContext* context = engine->context();
+  std::string BuildVecAdd(Engine* gdv_engine) {


The engine param is renamed to gdv_engine since it shadows the engine member variable in this class which introduces unnecessary confusion here

niyue · 2023-12-06T10:02:46Z

cpp/src/gandiva/engine.h

+  /// \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);


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.

niyue · 2023-12-06T14:24:11Z

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


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

Can we keep this? This was introduced by #6417 .

niyue · 2023-12-06T16:05:51Z

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.

niyue · 2023-12-07T01:36:35Z

@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

kou · 2023-12-07T08:07:46Z

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

niyue · 2023-12-07T09:18:31Z

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.

niyue · 2023-12-07T12:23:49Z

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

kou · 2023-12-08T00:10:32Z

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:

$ 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=01;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

niyue · 2023-12-08T09:44:45Z

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 🤦

niyue · 2023-12-08T15:30:19Z

cpp/src/gandiva/llvm_generator_test.cc

-  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";


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:

rename the function name in the test case to be more specific and avoid conflict

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, llvm/llvm-project#74903

…ilation.

…jectCache API.

…ing IR can be done.

kou

+1

cpp/src/gandiva/engine.cc

kou · 2023-12-21T06:56:59Z

@jorisvandenbossche Could you take a look at Cython changes?

@js8544 Do you want to review this before we merge this?

js8544

LGTM. I do have a question regarding the API changes.

cpp/src/gandiva/engine.cc

cpp/src/gandiva/engine.h

cpp/src/gandiva/llvm_generator.h

conbench-apache-arrow · 2024-01-05T00:24:21Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 83cba25.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

…ORC v2/LLJIT (apache#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. * This PR adds a new JIT linker option `JITLink` (https://llvm.org/docs/JITLink.html), which can be used together with `LLJIT`, for LLVM 14+ on Linux/macOS platform. It is turned off by default but could be turned on with environment variable `GANDIVA_USE_JIT_LINK` ### 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 `dump_ir`. If users would like to call `DumpIR` API of `Projector` and `Filter`, they have to set the `dump_ir` option first. * Closes: apache#37848 Authored-by: Yue Ni <niyue.com@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

…CJIT to ORC v2/LLJIT (apache#39098)" This reverts commit 83cba25.

…e from MCJIT to ORC v2/LLJIT (apache#39098)"" This reverts commit 2f34923.

…CJIT to ORC v2/LLJIT (apache#39098)" This reverts commit 83cba25.

github-actions bot added Component: C++ Component: C++ - Gandiva awaiting review Awaiting review labels Dec 6, 2023

niyue commented Dec 6, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Dec 6, 2023

niyue commented Dec 6, 2023

View reviewed changes

niyue force-pushed the feature/orc-v2-jit branch from f06a8cf to a2cbdd5 Compare December 6, 2023 09:58

niyue commented Dec 6, 2023

View reviewed changes

niyue force-pushed the feature/orc-v2-jit branch 4 times, most recently from 82ac6a2 to 83b1980 Compare December 6, 2023 14:21

niyue requested a review from wjones127 as a code owner December 6, 2023 14:21

github-actions bot added the Component: Python label Dec 6, 2023

niyue commented Dec 6, 2023

View reviewed changes

niyue force-pushed the feature/orc-v2-jit branch 3 times, most recently from d055df3 to 994d83c Compare December 7, 2023 05:55

niyue commented Dec 8, 2023

View reviewed changes

niyue added 9 commits December 20, 2023 21:52

Add a minimum atexit symbol lookup test for troubleshooting.

9f556ca

Remove the lookup test case used for troubleshooting.

5627ab4

Set several variables to be const.

e2d89b6

Add atexit symbol if it doesn't exist so that ASAN won't complain.

f5356e5

Switch to use JITLink for LLVM 9+ to see if it addresses the ASAN issue.

31a4b09

Make JITLink defaults to off, and add a benchmark for expression comp…

7462473

…ilation.

Rename configuration to dump_ir and return Status for the SetLLVMOb…

3213dbf

…jectCache API.

Add a gandiva Configuration class for the python binding so that dump…

7177d7f

…ing IR can be done.

Remove WIN64 macro since WIN32 could be enough to skip Windows.

db819dc

niyue force-pushed the feature/orc-v2-jit branch from d3deae2 to db819dc Compare December 20, 2023 13:53

kou approved these changes Dec 21, 2023

View reviewed changes

cpp/src/gandiva/engine.cc Show resolved Hide resolved

cpp/src/gandiva/engine.cc Outdated Show resolved Hide resolved

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Dec 21, 2023

js8544 approved these changes Dec 22, 2023

View reviewed changes

cpp/src/gandiva/engine.cc Show resolved Hide resolved

cpp/src/gandiva/engine.h Show resolved Hide resolved

cpp/src/gandiva/llvm_generator.h Show resolved Hide resolved

Require error_context for AsArrowResult API.

8b88f6d

niyue force-pushed the feature/orc-v2-jit branch from b484dfb to 8b88f6d Compare December 22, 2023 04:28

kou merged commit 83cba25 into apache:main Jan 4, 2024
30 of 32 checks passed

kou removed the awaiting merge Awaiting merge label Jan 4, 2024

lriggs added a commit to lriggs/arrow that referenced this pull request Mar 20, 2024

Revert "apacheGH-37848: [C++][Gandiva] Migrate LLVM JIT engine from M…

2f34923

…CJIT to ORC v2/LLJIT (apache#39098)" This reverts commit 83cba25.

ryantse added a commit to dremio/arrow that referenced this pull request Mar 30, 2024

Revert "Revert "apacheGH-37848: [C++][Gandiva] Migrate LLVM JIT engin…

f2aaf08

…e from MCJIT to ORC v2/LLJIT (apache#39098)"" This reverts commit 2f34923.

lriggs added a commit to lriggs/arrow that referenced this pull request Apr 5, 2024

Revert "apacheGH-37848: [C++][Gandiva] Migrate LLVM JIT engine from M…

6397aea

…CJIT to ORC v2/LLJIT (apache#39098)" This reverts commit 83cba25.

lriggs added a commit to lriggs/arrow that referenced this pull request May 10, 2024

Revert "apacheGH-37848: [C++][Gandiva] Migrate LLVM JIT engine from M…

343f70f

…CJIT to ORC v2/LLJIT (apache#39098)" This reverts commit 83cba25.

stevelorddremio pushed a commit to stevelorddremio/arrow that referenced this pull request Jun 14, 2024

Revert "apacheGH-37848: [C++][Gandiva] Migrate LLVM JIT engine from M…

c18d88c

…CJIT to ORC v2/LLJIT (apache#39098)" This reverts commit 83cba25.

lriggs added a commit to lriggs/arrow that referenced this pull request Sep 3, 2024

Revert "apacheGH-37848: [C++][Gandiva] Migrate LLVM JIT engine from M…

0e7db00

…CJIT to ORC v2/LLJIT (apache#39098)" This reverts commit 83cba25.

lriggs added a commit to lriggs/arrow that referenced this pull request Sep 6, 2024

Revert "apacheGH-37848: [C++][Gandiva] Migrate LLVM JIT engine from M…

1966018

…CJIT to ORC v2/LLJIT (apache#39098)" This reverts commit 83cba25.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT #39098

GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT #39098

niyue commented Dec 6, 2023 •

edited

Loading

github-actions bot commented Dec 6, 2023

niyue Dec 6, 2023

niyue Dec 6, 2023

niyue Dec 6, 2023

niyue Dec 6, 2023 •

edited

Loading

niyue Dec 6, 2023 •

edited

Loading

niyue Dec 6, 2023 •

edited

Loading

kou Dec 16, 2023

niyue commented Dec 6, 2023

niyue commented Dec 7, 2023

kou commented Dec 7, 2023

niyue commented Dec 7, 2023

niyue commented Dec 7, 2023

kou commented Dec 8, 2023

niyue commented Dec 8, 2023

niyue Dec 8, 2023 •

edited

Loading

kou left a comment

kou commented Dec 21, 2023

js8544 left a comment

conbench-apache-arrow bot commented Jan 5, 2024

GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT #39098

GH-37848: [C++][Gandiva] Migrate LLVM JIT engine from MCJIT to ORC v2/LLJIT #39098

Conversation

niyue commented Dec 6, 2023 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Dec 6, 2023

niyue Dec 6, 2023

Choose a reason for hiding this comment

niyue Dec 6, 2023

Choose a reason for hiding this comment

niyue Dec 6, 2023

Choose a reason for hiding this comment

niyue Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

niyue Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

niyue Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

kou Dec 16, 2023

Choose a reason for hiding this comment

niyue commented Dec 6, 2023

niyue commented Dec 7, 2023

kou commented Dec 7, 2023

niyue commented Dec 7, 2023

niyue commented Dec 7, 2023

kou commented Dec 8, 2023

niyue commented Dec 8, 2023

niyue Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

kou left a comment

Choose a reason for hiding this comment

kou commented Dec 21, 2023

js8544 left a comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Jan 5, 2024

niyue commented Dec 6, 2023 •

edited

Loading

niyue Dec 6, 2023 •

edited

Loading

niyue Dec 6, 2023 •

edited

Loading

niyue Dec 6, 2023 •

edited

Loading

niyue Dec 8, 2023 •

edited

Loading