Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ARROW-17790: [C++][Gandiva] Adapt to LLVM opaque pointer #14187

Merged
merged 7 commits into from Sep 22, 2022
Merged

ARROW-17790: [C++][Gandiva] Adapt to LLVM opaque pointer #14187

merged 7 commits into from Sep 22, 2022

Conversation

js8544
Copy link
Collaborator

@js8544 js8544 commented Sep 21, 2022

Starting from LLVM 13, LLVM IR has been shifting towards a unified opaque pointer type, i.e. pointers without pointee types. It has provided workarounds until LLVM 15. The temporary workarounds need to be replaced in order to support LLVM 15 and onwards. We need to supply the pointee type to the CreateGEP and CreateLoad methods.

For more background info, see https://llvm.org/docs/OpaquePointers.html and https://lists.llvm.org/pipermail/llvm-dev/2015-February/081822.html

Related issues:

https://issues.apache.org/jira/browse/ARROW-14363

https://issues.apache.org/jira/browse/ARROW-17728

https://issues.apache.org/jira/browse/ARROW-17775

@github-actions
Copy link

https://issues.apache.org/jira/browse/ARROW-17790

@js8544 js8544 force-pushed the jinshang/gandiva/fix_llvm15_deprecated_header branch from ee7eeef to 89076fa Compare September 21, 2022 06:26
@pitrou
Copy link
Member

pitrou commented Sep 21, 2022 •
edited

Is it possible to keep compatibility with pre-13 LLVM using some sort of compatibility wrappers?
cc @kou

@js8544
Copy link
Collaborator Author

js8544 commented Sep 21, 2022

@pitrou Sorry to bother you antoine, but I have a newbie question: How I can run gandiva related benchmarks with ursabot ?

Is it possible to keep compatibility with pre-13 LLVM using some sort of compatibility wrappers? cc @kou

@pitrou As far as I understand it is compatible with previous versions. Previously we used a helper function to let pointers duduce their own types. Now we supply their types directly.
Is there a way to check compatibility with old versions?

@pitrou
Copy link
Member

pitrou commented Sep 21, 2022

One simple possibility is to build and run benchmarks locally. Build Arrow C++ in release mode with -DARROW_BUILD_BENCHMARKS=ON. Then you'll get benchmarks as executable files in the build directory that you can run individually.

@pitrou
Copy link
Member

pitrou commented Sep 21, 2022

Is there a way to check compatibility with old versions?

Not sure. That might be tested on some of our nightly builds...

@pitrou
Copy link
Member

pitrou commented Sep 21, 2022

Hmm, judging by this error, we might have to bump the CLang version on the macOS C++ builder as well:
https://github.com/apache/arrow/actions/runs/3095675584/jobs/5010342464#step:9:1738

@kou @assignUser Is there a way to do that?

@pitrou
Copy link
Member

pitrou commented Sep 21, 2022

@github-actions crossbow submit -g cpp

@github-actions
Copy link

Revision: 5f40ace

Submitted crossbow builds: ursacomputing/crossbow @ actions-5342584c40

Task Status
test-alpine-linux-cpp Github Actions
test-build-cpp-fuzz Github Actions
test-conda-cpp Github Actions
test-conda-cpp-valgrind Azure
test-debian-10-cpp-amd64 Github Actions
test-debian-10-cpp-i386 Github Actions
test-debian-11-cpp-amd64 Github Actions
test-debian-11-cpp-i386 Github Actions
test-fedora-35-cpp Github Actions
test-ubuntu-18.04-cpp Github Actions
test-ubuntu-18.04-cpp-release Github Actions
test-ubuntu-18.04-cpp-static Github Actions
test-ubuntu-20.04-cpp Github Actions
test-ubuntu-20.04-cpp-17 Github Actions
test-ubuntu-20.04-cpp-bundled Github Actions
test-ubuntu-20.04-cpp-thread-sanitizer Github Actions
test-ubuntu-22.04-cpp Github Actions

@js8544
Copy link
Collaborator Author

js8544 commented Sep 21, 2022

@pitrou I update the ci scripts to use the clang version installed by brew. It yields many new warnings. Should I fix those warnings in this PR?

@pitrou
Copy link
Member

pitrou commented Sep 21, 2022

@js8544 If the warnings fail the build, then yes. Are you able to reproduce locally for faster iterations?

@js8544
Copy link
Collaborator Author

js8544 commented Sep 21, 2022

I just installed clang 15 on my machine and can reproduce now. Let me fix the warnings.

@js8544
Copy link
Collaborator Author

js8544 commented Sep 21, 2022 •
edited

@pitrou Most of these warnings are unrelated to Gandiva and I feed like fixing them in this PR makes it confusing for future maintenance.
Can I create another issue and PR that: 1. Use brew installed clang for mac in ci scripts 2. Fix clang-15 compatibility?

@pitrou
Copy link
Member

pitrou commented Sep 21, 2022

@js8544 Definitely!

@js8544
Copy link
Collaborator Author

js8544 commented Sep 21, 2022 •
edited

It turns out they have to be submited together because a successful clang-15 build requires this change from Gandiva. However I did create another issue to track it: https://issues.apache.org/jira/browse/ARROW-17805

@js8544 js8544 force-pushed the jinshang/gandiva/fix_llvm15_deprecated_header branch from e0cc3d0 to a960940 Compare September 21, 2022 16:04
@js8544
Copy link
Collaborator Author

js8544 commented Sep 21, 2022

We need to temporarily turn off gcsfs_test, s3fs_test, flight_internals_test and flight_test due to this issue: boostorg/container_hash#24

@pitrou
Copy link
Member

pitrou commented Sep 21, 2022 •
edited

@js8544 Can you show a snippet of the error(s) with boost?

Edit: looking at https://github.com/boostorg/config/pull/440/files, perhaps we can just define BOOST_NO_CXX98_FUNCTION_BASE before including boost?

@js8544
Copy link
Collaborator Author

js8544 commented Sep 21, 2022

In file included from /Users/jinshang/Projects/arrow/cpp/src/arrow/filesystem/gcsfs_test.cc:34:
In file included from /usr/local/include/boost/process.hpp:24:
In file included from /usr/local/include/boost/process/async_system.hpp:22:
In file included from /usr/local/include/boost/process/child.hpp:22:
In file included from /usr/local/include/boost/process/detail/execute_impl.hpp:24:
In file included from /usr/local/include/boost/process/detail/posix/executor.hpp:14:
In file included from /usr/local/include/boost/process/error.hpp:34:
In file included from /usr/local/include/boost/type_index.hpp:29:
In file included from /usr/local/include/boost/type_index/stl_type_index.hpp:47:
/usr/local/include/boost/container_hash/hash.hpp:132:33: error: no template named 'unary_function' in namespace 'std'; did you mean '__unary_function'?
struct hash_base : std::unary_function<T, std::size_t> {};
~~~~~^
/usr/local/opt/llvm/bin/../include/c++/v1/__functional/unary_function.h:46:1: note: '__unary_function' declared here
using __unary_function = __unary_function_keep_layout_base<_Arg, _Result>;
^
1 error generated.

@js8544
Copy link
Collaborator Author

js8544 commented Sep 21, 2022

@js8544 Can you show a snippet of the error(s) with boost?

Edit: looking at https://github.com/boostorg/config/pull/440/files, perhaps we can just define BOOST_NO_CXX98_FUNCTION_BASE before including boost?

Makes sense, I'll try it.

@js8544 js8544 force-pushed the jinshang/gandiva/fix_llvm15_deprecated_header branch from a960940 to f1de7e0 Compare September 21, 2022 16:30
@js8544 js8544 force-pushed the jinshang/gandiva/fix_llvm15_deprecated_header branch from f1de7e0 to 66353c9 Compare September 22, 2022 03:27
@js8544
Copy link
Collaborator Author

js8544 commented Sep 22, 2022

From https://github.com/apache/arrow/actions/runs/3099507453 and https://github.com/apache/arrow/actions/runs/3099507450 it seems to be working but the Mac build timed out. I force pushed with no change to trigger a rebuild.

@js8544
Copy link
Collaborator Author

js8544 commented Sep 22, 2022 •
edited

As far as I understand it is compatible with previous versions. Previously we used a helper function to let pointers duduce their own types. Now we supply their types directly.

Also from https://app.travis-ci.com/github/apache/arrow/jobs/583496235 it seems to be compatible with LLVM 10

kou
kou approved these changes Sep 22, 2022
View reviewed changes
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Thanks!

cpp/src/arrow/filesystem/gcsfs_test.cc Outdated
@@ -31,6 +31,7 @@
// We need BOOST_USE_WINDOWS_H definition with MinGW when we use
// boost/process.hpp. See BOOST_USE_WINDOWS_H=1 in
// cpp/cmake_modules/ThirdpartyToolchain.cmake for details.
#define BOOST_NO_CXX98_FUNCTION_BASE // ARROW-17805
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this to #20 (before // This boost/asio/...) to avoid confusing the above // We need ... comment target?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

cpp/src/arrow/filesystem/s3_test_util.cc Outdated
@@ -37,6 +37,7 @@
// We need BOOST_USE_WINDOWS_H definition with MinGW when we use
// boost/process.hpp. See BOOST_USE_WINDOWS_H=1 in
// cpp/cmake_modules/ThirdpartyToolchain.cmake for details.
#define BOOST_NO_CXX98_FUNCTION_BASE // ARROW-17805
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

cpp/src/gandiva/llvm_generator.cc
break;
case SelectionVector::MODE_UINT64:
arguments.push_back(types()->i64_ptr_type());
selection_vector_type = types()->i64_type();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not related to this pull request but could you also add break; here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

cpp/src/gandiva/llvm_generator.cc Outdated
@@ -378,7 +386,8 @@ Status LLVMGenerator::CodeGenExprValue(DexPtr value_expr, int buffer_count,
SetPackedBitValue(output_ref, loop_var, output_value->data());
} else if (arrow::is_primitive(output_type_id) ||
output_type_id == arrow::Type::DECIMAL) {
llvm::Value* slot_offset = CreateGEP(builder, output_ref, loop_var);
llvm::Value* slot_offset =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use auto here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

cpp/src/gandiva/llvm_generator.cc
slot = CreateGEP(builder, offsets_slot_ref, offsets_slot_index);
llvm::Value* offset_start = CreateLoad(builder, slot, "offset_start");
slot = builder->CreateGEP(types->i32_type(), offsets_slot_ref, offsets_slot_index);
llvm::Value* offset_start =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use auto here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

cpp/src/gandiva/llvm_generator.cc Outdated
slot = CreateGEP(builder, offsets_slot_ref, offsets_slot_index_next);
llvm::Value* offset_end = CreateLoad(builder, slot, "offset_end");
slot = builder->CreateGEP(types->i32_type(), offsets_slot_ref, offsets_slot_index_next);
llvm::Value* offset_end = builder->CreateLoad(types->i32_type(), slot, "offset_end");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use auto here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

cpp/src/gandiva/llvm_generator.cc Outdated
@@ -621,7 +634,8 @@ void LLVMGenerator::Visitor::Visit(const VectorReadVarLenValueDex& dex) {
// get the data from the data array, at offset 'offset_start'.
llvm::Value* data_slot_ref =
GetBufferReference(dex.DataIdx(), kBufferTypeData, dex.Field());
llvm::Value* data_value = CreateGEP(builder, data_slot_ref, offset_start);
llvm::Value* data_value =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use auto here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

cpp/src/gandiva/llvm_generator.cc Outdated
@@ -831,7 +845,7 @@ void LLVMGenerator::Visitor::Visit(const NullableInternalFuncDex& dex) {
result_ = BuildFunctionCall(native_function, arrow_return_type, &params);

// load the result validity and truncate to i1.
llvm::Value* result_valid_i8 = CreateLoad(builder, result_valid_ptr);
llvm::Value* result_valid_i8 = builder->CreateLoad(types->i8_type(), result_valid_ptr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use auto here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

cpp/src/gandiva/llvm_generator.cc Outdated
@@ -1278,8 +1294,8 @@ std::vector<llvm::Value*> LLVMGenerator::Visitor::BuildParams(
llvm::BasicBlock* saved_block = builder->GetInsertBlock();
builder->SetInsertPoint(entry_block_);

llvm::Value* holder =
generator_->LoadVectorAtIndex(arg_holder_ptrs_, holder_idx, "holder");
llvm::Value* holder = generator_->LoadVectorAtIndex(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use auto here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@js8544 js8544 force-pushed the jinshang/gandiva/fix_llvm15_deprecated_header branch from 92d0275 to 80f7750 Compare September 22, 2022 05:36
@js8544
Copy link
Collaborator Author

js8544 commented Sep 22, 2022 •
edited

@pitrou @kou Some builds are timing out. Any idea how to solve this?
Edit: example: https://github.com/apache/arrow/pull/14187/checks?check_run_id=8485942541

@kou
Copy link
Member

kou commented Sep 22, 2022

Could you increase timeout-minutes in https://github.com/apache/arrow/blob/master/.github/workflows/cpp.yml to 75 from 60 for now?

@pitrou
Copy link
Member

pitrou commented Sep 22, 2022

@github-actions crossbow submit -g cpp

@github-actions
Copy link

Revision: 266e915

Submitted crossbow builds: ursacomputing/crossbow @ actions-d354b31e2c

Task Status
test-alpine-linux-cpp Github Actions
test-build-cpp-fuzz Github Actions
test-conda-cpp Github Actions
test-conda-cpp-valgrind Azure
test-debian-10-cpp-amd64 Github Actions
test-debian-10-cpp-i386 Github Actions
test-debian-11-cpp-amd64 Github Actions
test-debian-11-cpp-i386 Github Actions
test-fedora-35-cpp Github Actions
test-ubuntu-18.04-cpp Github Actions
test-ubuntu-18.04-cpp-release Github Actions
test-ubuntu-18.04-cpp-static Github Actions
test-ubuntu-20.04-cpp Github Actions
test-ubuntu-20.04-cpp-17 Github Actions
test-ubuntu-20.04-cpp-bundled Github Actions
test-ubuntu-20.04-cpp-thread-sanitizer Github Actions
test-ubuntu-22.04-cpp Github Actions

@js8544
Copy link
Collaborator Author

js8544 commented Sep 22, 2022

@pitrou @kou Looks like it's now working as expected. A fully green checklist. Travis and crossbow failures are unrelated.

@pitrou
Copy link
Member

pitrou commented Sep 22, 2022

Thanks a lot @js8544 !

@pitrou pitrou merged commit 311fe3e into apache:master Sep 22, 2022
@ursabot
Copy link

ursabot commented Sep 22, 2022

Benchmark runs are scheduled for baseline = 43e66a9 and contender = 311fe3e. 311fe3e is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.41% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.28% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.32% ⬆️0.04%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 311fe3e8 ec2-t3-xlarge-us-east-2
[Finished] 311fe3e8 test-mac-arm
[Failed] 311fe3e8 ursa-i9-9960x
[Finished] 311fe3e8 ursa-thinkcentre-m75q
[Finished] 43e66a92 ec2-t3-xlarge-us-east-2
[Finished] 43e66a92 test-mac-arm
[Failed] 43e66a92 ursa-i9-9960x
[Finished] 43e66a92 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@js8544 js8544 mentioned this pull request Sep 26, 2022
Merged
kou pushed a commit that referenced this pull request Sep 27, 2022
@js8544
ARROW-17842: [C++][CI] Use Brew installed clang for MacOS verify-rc (#…
f277f2e 
…14236)

Same problem as #14187 (comment)

Authored-by: Jin Shang <shangjin1997@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
zagto pushed a commit to zagto/arrow that referenced this pull request Oct 7, 2022
@js8544 @zagto
ARROW-17790: [C++][Gandiva] Adapt to LLVM opaque pointer (apache#14187)
1aa26fe 
Starting from LLVM 13, LLVM IR has been shifting towards a unified opaque pointer type, i.e. pointers without pointee types. It has provided workarounds until LLVM 15. The temporary workarounds need to be replaced in order to support LLVM 15 and onwards. We need to supply the pointee type to the CreateGEP and CreateLoad methods.

For more background info, see https://llvm.org/docs/OpaquePointers.html and https://lists.llvm.org/pipermail/llvm-dev/2015-February/081822.html

Related issues:

https://issues.apache.org/jira/browse/ARROW-14363

https://issues.apache.org/jira/browse/ARROW-17728

https://issues.apache.org/jira/browse/ARROW-17775

Lead-authored-by: Jin Shang <shangjin1997@gmail.com>
Co-authored-by: jinshang <jinshang@tencent.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@js8544 js8544 deleted the jinshang/gandiva/fix_llvm15_deprecated_header branch October 11, 2022 08:43
fatemehp pushed a commit to fatemehp/arrow that referenced this pull request Oct 17, 2022
@js8544 @fatemehp
ARROW-17790: [C++][Gandiva] Adapt to LLVM opaque pointer (apache#14187)
048d212 
Starting from LLVM 13, LLVM IR has been shifting towards a unified opaque pointer type, i.e. pointers without pointee types. It has provided workarounds until LLVM 15. The temporary workarounds need to be replaced in order to support LLVM 15 and onwards. We need to supply the pointee type to the CreateGEP and CreateLoad methods.

For more background info, see https://llvm.org/docs/OpaquePointers.html and https://lists.llvm.org/pipermail/llvm-dev/2015-February/081822.html

Related issues:

https://issues.apache.org/jira/browse/ARROW-14363

https://issues.apache.org/jira/browse/ARROW-17728

https://issues.apache.org/jira/browse/ARROW-17775

Lead-authored-by: Jin Shang <shangjin1997@gmail.com>
Co-authored-by: jinshang <jinshang@tencent.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
fatemehp pushed a commit to fatemehp/arrow that referenced this pull request Oct 17, 2022
@js8544 @fatemehp
ARROW-17842: [C++][CI] Use Brew installed clang for MacOS verify-rc (a…
43e05ac 
…pache#14236)

Same problem as apache#14187 (comment)

Authored-by: Jin Shang <shangjin1997@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
This was referenced Sep 22, 2022
Closed
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kou kou kou approved these changes

Assignees
No one assigned
Labels
Component: C++ - Gandiva Component: C++ Component: FlightRPC
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@js8544 @pitrou @kou @ursabot