You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/05/27 19:17:31 UTC

[GitHub] [arrow] kiszk opened a new pull request #7295: ARROW-8968: [C++][gandiva] set data layout for pre-compiled IR to llvm::module

kiszk opened a new pull request #7295:
URL: https://github.com/apache/arrow/pull/7295


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


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kiszk edited a comment on pull request #7295: ARROW-8968: [C++][Gandiva] set data layout for pre-compiled IR to llvm::module

Posted by GitBox <gi...@apache.org>.
kiszk edited a comment on pull request #7295:
URL: https://github.com/apache/arrow/pull/7295#issuecomment-635351048


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


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] github-actions[bot] commented on pull request #7295: ARROW-8968: [C++][gandiva] set data layout for pre-compiled IR to llvm::module

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7295:
URL: https://github.com/apache/arrow/pull/7295#issuecomment-634893457


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


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kiszk commented on pull request #7295: ARROW-8968: [C++][Gandiva] set data layout for pre-compiled IR to llvm::module

Posted by GitBox <gi...@apache.org>.
kiszk commented on pull request #7295:
URL: https://github.com/apache/arrow/pull/7295#issuecomment-635351048


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


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] wesm commented on a change in pull request #7295: ARROW-8968: [C++][Gandiva] set data layout for pre-compiled IR to llvm::module

Posted by GitBox <gi...@apache.org>.
wesm commented on a change in pull request #7295:
URL: https://github.com/apache/arrow/pull/7295#discussion_r431454890



##########
File path: cpp/src/gandiva/engine.cc
##########
@@ -150,6 +152,27 @@ Status Engine::Make(const std::shared_ptr<Configuration>& conf,
   return Status::OK();
 }
 
+static void setDataLayout(llvm::Module* module) {

Review comment:
       nit: SetDataLayout

##########
File path: cpp/src/gandiva/engine.cc
##########
@@ -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();

Review comment:
       nit: use lower_with_underscores for local variables

##########
File path: cpp/src/gandiva/engine.cc
##########
@@ -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();
+  std::string errorMessage;
+  auto target = llvm::TargetRegistry::lookupTarget(targetTriple, errorMessage);
+  if (!target) {
+    return;
+  }
+
+  std::string cpu(llvm::sys::getHostCPUName());
+  llvm::SubtargetFeatures features;
+  llvm::StringMap<bool> hostFeatures;
+
+  if (llvm::sys::getHostCPUFeatures(hostFeatures))
+    for (auto& f : hostFeatures) features.AddFeature(f.first(), f.second);

Review comment:
       nit: please use braces




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kiszk edited a comment on pull request #7295: ARROW-8968: [C++][Gandiva] set data layout for pre-compiled IR to llvm::module

Posted by GitBox <gi...@apache.org>.
kiszk edited a comment on pull request #7295:
URL: https://github.com/apache/arrow/pull/7295#issuecomment-635036852


   Regarding the licence, I will add a link to the file at https://github.com/llvm/llvm-project/blob/master/mlir/LICENSE.TXT.
   Could you please let me know if we need to create a separate file as https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/bpacking_default.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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] wesm commented on pull request #7295: ARROW-8968: [C++][Gandiva] set data layout for pre-compiled IR to llvm::module

Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7295:
URL: https://github.com/apache/arrow/pull/7295#issuecomment-635354064


   @kiszk yes I think that's a good idea cc @kou 


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] wesm closed pull request #7295: ARROW-8968: [C++][Gandiva] set data layout for pre-compiled IR to llvm::module

Posted by GitBox <gi...@apache.org>.
wesm closed pull request #7295:
URL: https://github.com/apache/arrow/pull/7295


   


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kiszk commented on a change in pull request #7295: ARROW-8968: [C++][Gandiva] set data layout for pre-compiled IR to llvm::module

Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #7295:
URL: https://github.com/apache/arrow/pull/7295#discussion_r431566905



##########
File path: cpp/src/gandiva/engine.cc
##########
@@ -150,6 +152,27 @@ Status Engine::Make(const std::shared_ptr<Configuration>& conf,
   return Status::OK();
 }
 
+static void setDataLayout(llvm::Module* module) {

Review comment:
       Thank you, addressed all of 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kiszk commented on pull request #7295: ARROW-8968: [C++][Gandiva] set data layout for pre-compiled IR to llvm::module

Posted by GitBox <gi...@apache.org>.
kiszk commented on pull request #7295:
URL: https://github.com/apache/arrow/pull/7295#issuecomment-635036852


   Regarding the licence, I will add a link to the file at https://github.com/llvm/llvm-project/blob/master/mlir/LICENSE.TXT.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org