Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15718 )
Change subject: IMPALA-9645 Port LLVM codegen to adapt aarch64 ...................................................................... Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/15718/3/be/src/codegen/codegen-anyval.cc File be/src/codegen/codegen-anyval.cc: http://gerrit.cloudera.org:8080/#/c/15718/3/be/src/codegen/codegen-anyval.cc@329 PS3, Line 329: // Lowered type is of form { {i8}, [15 x i8], {i128} }. Get the i128 value and Can you reorder so that the comment is next to the relevant code. I.e. #ifdef __aarch64__ // On aarch64, the lowered type is of form { {i8}, {i128} }. No padding add. uint32_t idxs[] = {1, 0}; #else // On x86-64, the lowered type is of form { {i8}, [15 x i8], {i128} }. Get the i128 value and // truncate it to the correct size. (The {i128} corresponds to the union of the // different width int types.) uint32_t idxs[] = {2, 0}; #endif http://gerrit.cloudera.org:8080/#/c/15718/3/be/src/exprs/scalar-expr.cc File be/src/exprs/scalar-expr.cc: http://gerrit.cloudera.org:8080/#/c/15718/3/be/src/exprs/scalar-expr.cc@405 PS3, Line 405: // add conversion function here which convert val type between aarch64 and x86 This does not seem like an elegant solution. Why not change CreateCall() so that it generates the right call? I think this code should belong in CodegenAnyval in any case http://gerrit.cloudera.org:8080/#/c/15718/3/be/src/exprs/scalar-expr.cc@408 PS3, Line 408: const ColumnType& type, llvm::Value* result_val, LlvmBuilder& builder,std::string name){ Can you run clang-format-diff on this patch to fix the formatting? See https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536 There are a bunch of minor formatting issues in this function. -- To view, visit http://gerrit.cloudera.org:8080/15718 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3f30ee84ea9bf5245da88154632bb69079103d11 Gerrit-Change-Number: 15718 Gerrit-PatchSet: 3 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 21 Apr 2020 18:16:45 +0000 Gerrit-HasComments: Yes
