Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20333 )
Change subject: IMPALA-12314: (Addendum) Fix building on aarch64 ...................................................................... Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/20333/2/be/src/codegen/CMakeLists.txt File be/src/codegen/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/20333/2/be/src/codegen/CMakeLists.txt@100 PS2, Line 100: COMPILE_TO_IR_C_ARRAY(${IR_O1_C_FILE} impala_llvm_o1_ir -O1 ${PLATFORM_SPECIFIC_FLAGS}) : COMPILE_TO_IR_C_ARRAY(${IR_O2_C_FILE} impala_llvm_o2_ir -O2 ${PLATFORM_SPECIFIC_FLAGS}) : COMPILE_TO_IR_C_ARRAY(${IR_Os_C_FILE} impala_llvm_os_ir -Os ${PLATFORM_SPECIFIC_FLAGS}) : COMPILE_TO_IR_C_ARRAY(${LEGACY_AVX_IR_C_FILE} impala_legacy_avx_llvm_ir -O1 ${LEGACY_AVX_SPECIFIC_FLAGS}) > One small thing here is that the way we are handling the flags won't deal w This works with ARGN, no cmake_parse_arguments needed. http://gerrit.cloudera.org:8080/#/c/20333/2/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/20333/2/be/src/codegen/llvm-codegen.cc@277 PS2, Line 277: module_ir = llvm::StringRef( : reinterpret_cast<const char*>(impala_llvm_o1_ir), impala_llvm_o1_ir_len); : } else if (FLAGS_llvm_ir_opt == "O2") { : module_ir = llvm::StringRef( : reinterpret_cast<const char*>(impala_llvm_o2_ir), impala_llvm_o2_ir_len); : } else if (FLAGS_llvm_ir_opt == "Os") { : module_ir = llvm::StringRef( : reinterpret_cast<const char*>(impala_llvm_os_ir), impala_llvm_os_ir_len); : } else { : CHECK(false) << "llvm_ir_opt flag invalid; try O1, O2, or Os."; : } : #if __x86_64__ : // By default, Impala now requires AVX2 support, but the enable_legacy_avx_support : // flag can allow running on AVX machines. The minimum requirement must have already : // been enforced prior to this call, so this only needs to select the appropriate : // LLVM IR to use. : if (IsCPUFeatureEnabled(CpuInfo::AVX2)) { : // Use the default IR that supports AVX2 : module_name = "Impala IR with AVX2 support"; : } else if (FLAGS_enable_legacy_avx_support && IsCPUFeatureEnabled(CpuInfo::AVX)) { : // If there is no AVX but legacy mode is enabled, use legacy IR with AVX support : module_ir = llvm::StringRef( : reinterpret_cast<const char*>(impala_legacy_avx_llvm_ir), : impala_legacy_avx_llvm_ir_len); : module_name = "Legacy Impala IR with AVX support"; : } else { : // This should have been enforced earlier. : CHECK(false) << "CPU is missing AVX/AVX2 support"; : } : #endif : : unique_ptr<llvm::MemoryBuffer> module_ir_buf( : llvm::MemoryBuffer::getMemBuffer(module_ir, "", false)); : unique_ptr<llvm::Module> loaded_module; : Status status = (*codegen)->LoadModuleFromMemory(move(module_ir_buf), : > Nit: I wonder if we can consolidate the x86_64 logic into a single chunk. S Done -- To view, visit http://gerrit.cloudera.org:8080/20333 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie7223b82c606c91f98edbf583dfab2f9fe7ddbfd Gerrit-Change-Number: 20333 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Wed, 09 Aug 2023 16:46:32 +0000 Gerrit-HasComments: Yes
