Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/20333 )
Change subject: IMPALA-12314: (Addendum) Fix building on aarch64 ...................................................................... Patch Set 2: (2 comments) Thanks for looking into this 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 with multiple flags properly. If PLATFORM_SPECIFIC_FLAGS was two flags as a single string, they will be seen as a single argument with a space for clang and clang fails. If PLATFORM_SPECIFIC_FLAGS is a list of flags (i.e. no quotes -mavx -otherflag), the COMPILE_TO_IR_C_ARRAY would only understand the first flag and ignore the rest. Handling this would probably be easier using cmake_parse_arguments. It doesn't really impact us right now, so we could defer that. 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: #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 : #endif : if (FLAGS_llvm_ir_opt == "O1") { : 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__ : 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"; : } : #else : // Non-x86_64 always use the default IR : module_name = "Impala IR"; : #endif Nit: I wonder if we can consolidate the x86_64 logic into a single chunk. Something like this: // Platform agnostic stuff if (FLAGS_llvm_ir_opt = "O1") { ... } else ... ... all the other conditions ... } module_name = "Impala IR"; #if __x86_64__ if (IsCpuFeatureEnabled(CpuInfo::AVX2)) { // Customize the module name for clarity module_name = "Impala IR with AVX2 support"; } else if (FLAGS_enable_legacy_avx_support && IsCPUFeatureEnabled(CpuInfo::AVX)) { module_ir = ... module_name = "Legacy Impala IR with AVX support"; } else { // This should have been enforced earlier. CHECK(false) << "CPU is missing AVX/AVX2 support"; } #endif -- 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: 2 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 00:45:23 +0000 Gerrit-HasComments: Yes
