GH-51245: [C++][Compute][Gandiva] Add support for LLVM 23.1 - #51266
GH-51245: [C++][Compute][Gandiva] Add support for LLVM 23.1#51266HuaHuaY wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The LLVM>=23 JITLink lambda in engine.cc does not capture memory_manager, which should fail to compile.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Arrow C++/Gandiva compatibility updates for LLVM 23.1, including JITLink integration adjustments, fixing scalar cast template instantiation for date/timestamp types, and ensuring generated/embedded bitcode is host-appropriate at runtime.
Changes:
- Add LLVM 23.1 to supported versions and relax Gandiva conda env LLVM pinning.
- Update Gandiva JITLink configuration and strip build-target function attributes from embedded bitcode before JIT-compiling.
- Fix
CastImpltemplate constraints for date/timestamp casts and add targeted unit tests; removestaticfrom several header template helpers to avoid LLVM/Clang-related linkage issues.
File summaries
| File | Description |
|---|---|
| cpp/src/gandiva/engine.cc | Adjust JITLink layer creation for LLVM 23+ and remove target CPU/features attrs from embedded bitcode before linking. |
| cpp/src/arrow/util/future.h | Remove static from a header template helper to avoid problematic linkage/instantiation behavior. |
| cpp/src/arrow/util/bit_block_counter.h | Remove static from header template visitors. |
| cpp/src/arrow/util/async_generator.h | Remove static from header template generator factories. |
| cpp/src/arrow/scalar.cc | Fix date/timestamp cast template constraints to match type-based dispatch. |
| cpp/src/arrow/scalar_test.cc | Add unit tests covering Date32/Date64/Timestamp CastTo paths. |
| cpp/src/arrow/compute/kernels/vector_hash.cc | Attempt to suppress unused warnings in a templated observer overload. |
| cpp/src/arrow/compute/kernels/codegen_internal.h | Remove static from header template helpers used in kernel codegen. |
| cpp/src/arrow/compute/function_internal.h | Remove static from a header template specialization for list scalar conversion. |
| cpp/src/arrow/compute/exec.cc | Remove an unused internal helper template (no longer referenced). |
| cpp/CMakeLists.txt | Add LLVM 23.1 to ARROW_LLVM_VERSIONS. |
| ci/conda_env_gandiva.txt | Unpin llvmdev to allow LLVM 23.x in the Gandiva conda environment. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| template <class Index> | ||
| void ObserveNullNotFound(Index index) { | ||
| [[maybe_unused]] void ObserveNullNotFound(Index index) { |
There was a problem hiding this comment.
What's the resolution here exactly?
There was a problem hiding this comment.
I added [[maybe_unused]] to avoid -Wunused-template, which differs from Copilot's suggestion. The issue reported by Copilot does not prevent compilation (I didn't even see a warning in my local environment; I assume it's because the template function isn't instantiated anywhere). Without the changes we will get:
[build] [3/102] Building CXX object src/arrow/CMakeFiles/arrow_compute_core.dir/compute/kernels/vector_hash.cc.o
[build] FAILED: [code=1] src/arrow/CMakeFiles/arrow_compute_core.dir/compute/kernels/vector_hash.cc.o
[build] sccache /nix/store/s8j4m3bnnqa6lznxd9c4j3043q3rj8my-clang-wrapper-23.1.0/bin/clang++ -DARROW_EXTRA_ERROR_CONTEXT -DARROW_HAVE_NEON -DARROW_WITH_TIMING_TESTS -I/Users/huahua/github/arrow/cpp/out/build/ninja-debug-gandiva/src -I/Users/huahua/github/arrow/cpp/src -fno-aligned-new -Qunused-arguments -fcolor-diagnostics -Wall -Wextra -Wdocumentation -DARROW_WARN_DOCUMENTATION -Wshorten-64-to-32 -Wno-missing-braces -Wno-unused-parameter -Wno-constant-logical-operand -Wno-return-stack-address -Wdate-time -Wno-unknown-warning-option -Wno-pass-failed -march=armv8-a -g -Werror -O0 -ggdb -std=c++20 -arch arm64 -isysroot /nix/store/52kwxj456mb1ygjn9nkgm93a17si3ks4-apple-sdk-14.4/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk -mmacosx-version-min=14.0 -fPIC -MD -MT src/arrow/CMakeFiles/arrow_compute_core.dir/compute/kernels/vector_hash.cc.o -MF src/arrow/CMakeFiles/arrow_compute_core.dir/compute/kernels/vector_hash.cc.o.d -o src/arrow/CMakeFiles/arrow_compute_core.dir/compute/kernels/vector_hash.cc.o -c /Users/huahua/github/arrow/cpp/src/arrow/compute/kernels/vector_hash.cc
[build] /Users/huahua/github/arrow/cpp/src/arrow/compute/kernels/vector_hash.cc:139:8: error: unused function template 'ObserveNullNotFound' [-Werror,-Wunused-template]
[build] 139 | void ObserveNullNotFound(Index index) {
[build] | ^~~~~~~~~~~~~~~~~~~
[build] 1 error generated.
[build] ninja: build stopped: subcommand failed.
HuaHuaY
left a comment
HuaHuaY
left a comment
There was a problem hiding this comment.
To assist the reviewer with the review process, I submit some explanatory notes.
| template <typename T, typename VisitFunc, typename NullFunc> | ||
| requires std::is_void_v<std::invoke_result_t<VisitFunc, typename GetViewType<T>::T>> | ||
| static void VisitArrayValuesInline(const ArraySpan& arr, VisitFunc&& valid_func, | ||
| void VisitArrayValuesInline(const ArraySpan& arr, VisitFunc&& valid_func, |
There was a problem hiding this comment.
LLVM 23 added -Wunused-template to -Wall.
https://releases.llvm.org/23.1.0/tools/clang/docs/ReleaseNotes.html
-Wunused-template is now part of -Wunused (which is enabled by -Wall). It diagnoses unused function and variable templates with internal linkage, which in a header is a latent ODR hazard. It can be disabled with -Wno-unused-template. (#202945)
I don't think there's any harm in removing static. Template functions are inherently similar to inline functions, and ODR issues won't arise.
| const FunctionOptions* options_; | ||
| }; | ||
|
|
||
| template <typename ExecutorType, |
There was a problem hiding this comment.
It's a template function which doesn't have any callers.
| // date to date | ||
| template <typename To> | ||
| enable_if_t<std::is_same<To, Date64Scalar>::value, Result<std::shared_ptr<Scalar>>> | ||
| enable_if_t<std::is_same<To, Date64Type>::value, Result<std::shared_ptr<Scalar>>> |
There was a problem hiding this comment.
This is another issue exposed by -Wunused-template. The template instantiation type was incorrect here, causing the function that was supposed to execute this path to fall through to the "NotImplemented" exception instead. A unit test has been added.
| arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected, | ||
| const std::string& error_context) { | ||
| if (!expected) { | ||
| // NOTE: llvm::handleAllErrors() fails linking with RTTI-disabled LLVM builds |
There was a problem hiding this comment.
This comment was supposed to be moved here from line 425 during a previous code refactoring, but it was overlooked.
| if (maybe_use_jit_link.ok()) { | ||
| ARROW_ASSIGN_OR_RAISE(static auto memory_manager, CreateMemmoryManager()); | ||
| # if LLVM_VERSION_MAJOR >= 21 | ||
| # if LLVM_VERSION_MAJOR >= 23 |
There was a problem hiding this comment.
llvm/llvm-project#192214 added one more parameter.
|
|
||
| // Built-in bitcode is JIT-compiled on the runtime host. Do not retain the target | ||
| // selected by Clang when the bitcode was built. | ||
| RemoveBuildTargetAttributes(*src_ir_module); |
There was a problem hiding this comment.
The requirements for inlining have become stricter in LLVM 23.
https://releases.llvm.org/23.1.0/docs/ReleaseNotes.html#changes-to-the-llvm-ir
alwaysinline no longer bypasses inlining compatibility checks based on target features. Inlining will only be performed if it is safe to do so.
github-actions
Bot
added
awaiting committer review
There was a problem hiding this comment.
🟡 Changes recommended
There are a few concrete build/CI and warning-suppression issues (typoed helper name, incorrect [[maybe_unused]] placement, and unbounded LLVM conda dependency) that should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
cpp/src/gandiva/engine.cc:135
- The NOTE mentions
llvm::handleAllErrors()but this function doesn't call it directly (it callsllvm::toString(...)). Rewording avoids confusion about what is actually being avoided here.
cpp/src/gandiva/engine.cc:204 - Typo in helper name:
CreateMemmoryManagershould beCreateMemoryManager(double 'm' in Memory). This is easy to miss and will propagate to the call site below.
This issue also appears on line 218 of the same file.
cpp/src/gandiva/engine.cc:222
- Call site should match the corrected helper name (
CreateMemoryManager).
});
# else
ARROW_ASSIGN_OR_RAISE(static auto memory_manager, CreateMemmoryManager());
# if LLVM_VERSION_MAJOR >= 21
jit_builder.setObjectLinkingLayerCreator([&](llvm::orc::ExecutionSession& ES) {
cpp/src/arrow/compute/kernels/vector_hash.cc:141
[[maybe_unused]]is applied to the function here, but the unused entity is the parameterindex. If-Wunused-parameteris enabled, this won't suppress the warning. Prefer applying the attribute to the parameter (or useARROW_UNUSED(index)in the body).
template <class Index>
[[maybe_unused]] void ObserveNullNotFound(Index index) {
ARROW_LOG(FATAL) << "ObserveNullNotFound without err_status should not be called";
}
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The changes are consistent with the stated LLVM 23.1 support goal, are localized, and include targeted unit coverage for the fixed scalar casting behavior.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness/build issues in the updated code paths (notably unused-warning suppression and external bitcode handling consistency) that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
cpp/src/arrow/compute/kernels/vector_hash.cc:140
[[maybe_unused]]is applied to the function, but the warning being addressed is the unused parameterindex. This doesn’t suppress-Wunused-parameter(and may not fix the build with-Werror). Apply the attribute to the parameter instead (or explicitly ignore it).
template <class Index>
[[maybe_unused]] void ObserveNullNotFound(Index index) {
ARROW_LOG(FATAL) << "ObserveNullNotFound without err_status should not be called";
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
| auto llvm_memory_buffer_ref = AsLLVMMemoryBuffer(*buffer); | ||
| auto module_or_error = llvm::parseBitcodeFile(llvm_memory_buffer_ref, *context()); | ||
| ARROW_RETURN_NOT_OK(VerifyAndLinkModule(*module_, std::move(module_or_error))); | ||
| ARROW_ASSIGN_OR_RAISE( | ||
| auto src_ir_module, | ||
| AsArrowResult(module_or_error, "Failed to verify and link module: ")); | ||
| ARROW_RETURN_NOT_OK(VerifyAndLinkModule(*module_, std::move(src_ir_module))); |
There was a problem hiding this comment.
This comment by Copilot is wrong, isn't it? Or should we actually force the precompiled bitcode's build target attributes to the same ones as the function registry?
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
LGTM except for the latest Copilot comment which I'm not sure about
I'm also not sure about that whether we should modify the build target attributes of external precompiled bitcode. But even if we disregard this Copilot recommendation, the only difference lies in whether LLVM decides to inline the code or not. It does not fail our unit tests. |
There was a problem hiding this comment.
🟢 Approval recommended
The changes are targeted for LLVM 23.1 compatibility, include a Gandiva regression test and Arrow scalar cast tests, and the modified logic is internally consistent with the surrounding code.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
| llvm::cantFail(std::move(error)); | ||
| } | ||
|
|
||
| void AddNativeBoolZExtAttrs(llvm::Function& function) { |
There was a problem hiding this comment.
@pitrou I have pushed a new commit to fix the CI failure. An optimization in LLVM 23 exposed another issue in our code. llvm/llvm-project#178977
When the LLVM JIT calls a function compiled by GCC that takes a bool argument, we inform LLVM that the parameter type is i1. Consequently, LLVM doesn't zero out the upper bits of the argument without zeroext attr; however, GCC assumes the upper bits of a bool are zero, resulting in the generation of incorrect code.
LLVM 22 did not fail the tests because, when calculating the i1 value, it used the sequence icmp ne (and X, 1), 0, which happened to clear the high bits of the register. However, after LLVM 23 optimized this process to trunc X to i1 in the mentioned PR, that side effect—which we relied upon—was eliminated.
For example,
GCC may compile if(!value) as the equivalent of if((value ^ 1) != 0), assuming that a bool argument is normalized to 0 or 1. Suppose a bitmap byte contains 3 (0b11) and LLVM extracts bit 0 as an i1 value before passing it to a GCC-compiled function with a corresponding bool parameter.
- With LLVM 22, code generation for
icmp ne (and 3, 1), 0happened to pass the canonical value1, so(1 ^ 1) != 0evaluated to false. - With LLVM 23, this is optimized to
trunc 3 to i1. The LLVM result is correctlyi1 true, but withoutzeroext, the physical register passed to GCC may still contain3, so(3 ^ 1) != 0evaluates to true.
|
|
||
| namespace { | ||
|
|
||
| void CopyZExtAttrs(const llvm::Function& function, llvm::CallBase& call) { |
There was a problem hiding this comment.
According to https://llvm.org/docs/LangRef.html, we should also handle other attributes, but I would prefer to leave that for a separate PR later if someone needs.
ABI attributes must be specified both at the function declaration/definition and call-site, otherwise the behavior may be undefined. ABI attributes cannot be safely dropped.
There was a problem hiding this comment.
🔵 Needs a closer look
[[maybe_unused]] is applied to the function (not the unused parameter) and may not prevent -Wunused-parameter failures under warning-as-error builds.
Review details
Suppressed comments (1)
cpp/src/arrow/compute/kernels/vector_hash.cc:140
[[maybe_unused]]is applied to the function, not the unused parameter, so it won’t silence-Wunused-parameterwarnings forindex. Mark the parameter itself (or omit the parameter name) instead.
template <class Index>
[[maybe_unused]] void ObserveNullNotFound(Index index) {
ARROW_LOG(FATAL) << "ObserveNullNotFound without err_status should not be called";
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
@github-actions crossbow submit conda |
|
Revision: 6e94005 Submitted crossbow builds: ursacomputing/crossbow @ actions-dd3019d7bb
|
|
@dmitry-chirkov-dremio @lriggs @akravchukdremio @xxlaykxx Could you review this? |
|
My AI provided two pieces of feedback, and I agree with its points:
I will submit another commit shortly. |
LLVM 7, really? That's so old we certainly don't care. Side note: we should perhaps trim the range of LLVM versions we support. |
|
@github-actions crossbow submit conda |
There was a problem hiding this comment.
🟢 Approval recommended
The changes are targeted, include new unit coverage for the behavioral fixes, and the remaining feedback is a minor naming typo in a local helper.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
cpp/src/gandiva/engine.cc:221
- Typo in the helper name
CreateMemmoryManager(double 'm') makes the intent less clear and propagates to the call site. Consider renaming it toCreateMemoryManagerwhile touching this code path.
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Revision: f59ad59 Submitted crossbow builds: ursacomputing/crossbow @ actions-2350c28142
|
There was a problem hiding this comment.
🔵 Needs a closer look
It changes low-level LLVM JIT/ABI behavior across multiple LLVM-version code paths, which is correctness-sensitive and should receive final human review with real-world LLVM 23.1 builds.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
There is at least one concrete fix needed in the updated code (the [[maybe_unused]] placement in vector_hash.cc doesn’t apply to the unused parameter and should be adjusted).
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
cpp/src/arrow/compute/kernels/vector_hash.cc:141
[[maybe_unused]]is applied to the member function, but the unused entity here is theindexparameter; this won’t silence-Wunused-parameter(and also doesn’t affect any warnings since this overload is called). Move the attribute to the parameter (and consider doing the same for theObserveNullNotFound(Index, Status*)overload, which also doesn’t useindex).
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
Rationale for this change
Add support for LLVM 23.1.
What changes are included in this PR?
staticfrom some template functions in header files.CastImpltemplate instantiation aboutDateandTimestampdata types, and add a unit test.zeroextattr when LLVM JIT passesi1to GCC-compiled functions.Are these changes tested?
Yes.
Are there any user-facing changes?
Yes.