ARROW-8968: [C++][Gandiva] set data layout for pre-compiled IR to llvm::module #7295
Conversation
kou
changed the title
There was a problem hiding this comment.
Most of this code comes from https://reviews.llvm.org/D80130 by @imaihal in llvm.
I presume this code is covered by the LLVM license, do we need to put that somewhere?
cpp/src/gandiva/engine.cc
Outdated
| @@ -150,6 +152,27 @@ Status Engine::Make(const std::shared_ptr<Configuration>& conf, | |||
| return Status::OK(); | |||
| } | |||
|
|
|||
| static void setDataLayout(llvm::Module* module) { | |||
There was a problem hiding this comment.
Thank you, addressed all of them.
cpp/src/gandiva/engine.cc
Outdated
| @@ -150,6 +152,27 @@ Status Engine::Make(const std::shared_ptr<Configuration>& conf, | |||
| return Status::OK(); | |||
| } | |||
|
|
|||
| static void setDataLayout(llvm::Module* module) { | |||
| auto targetTriple = llvm::sys::getDefaultTargetTriple(); | |||
There was a problem hiding this comment.
nit: use lower_with_underscores for local variables
cpp/src/gandiva/engine.cc
Outdated
| llvm::StringMap<bool> hostFeatures; | ||
|
|
||
| if (llvm::sys::getHostCPUFeatures(hostFeatures)) | ||
| for (auto& f : hostFeatures) features.AddFeature(f.first(), f.second); |
|
Regarding the licence, I will add a link to the file at https://github.com/llvm/llvm-project/blob/master/mlir/LICENSE.TXT. |
|
@kiszk see https://github.com/apache/arrow/blob/master/LICENSE.txt#L1071 Perhaps change "3rdparty dependency LLVM is statically linked in certain binary distributions." to "3rdparty dependency LLVM is statically linked in certain binary distributions. Additionally some sections of source code have been derived from sources in LLVM and have been clearly labeled as such." |
|
@wesm, thank you. I will update it. Another question. I have just realized the LLVM license is included in the file https://github.com/apache/arrow/blob/master/LICENSE.txt. LLVM changes its license from LLVM9 as Apache2 with LLVM exception. Should we update the file https://github.com/apache/arrow/blob/master/LICENSE.txt#L1074-L1141 when #7280 is merged? |
…m::module This PR explicitly sets data layout for pre-compiled IR to `llvm:module`. Without the PR, the following warning message is shown at link time of gandiva. Most of this code comes from https://reviews.llvm.org/D80130 by @imaihal in llvm. ``` ~/arrow/cpp/src/gandiva$ ../../build/debug/gandiva-binary-test -V Running main() from /home/ishizaki/arrow/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest_main.cc [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from TestBinary [ RUN ] TestBinary.TestSimple warning: Linking two modules of different data layouts: 'precompiled' is 'E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-a:8:16-n32:64' whereas 'codegen' is 'E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-v128:64-a:8:16-n32:64' [ OK ] TestBinary.TestSimple (41 ms) [----------] 1 test from TestBinary (41 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (41 ms total) [ PASSED ] 1 test. ``` Closes apache#7295 from kiszk/ARROW-8968 Authored-by: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Signed-off-by: Wes McKinney <wesm+git@apache.org>
This PR explicitly sets data layout for pre-compiled IR to
llvm:module. Without the PR, the following warning message is shown at link time of gandiva.Most of this code comes from https://reviews.llvm.org/D80130 by @imaihal in llvm.