Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Modify IRBuilder to use LLVM's CPU features ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen-test.cc File be/src/codegen/llvm-codegen-test.cc: http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen-test.cc@467 PS2, Line 467: CpuInfo::EnableFeature(CpuInfo::SSE4_2, false); Do we still need to toggle CpuInfo? http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.h@737 PS2, Line 737: current_cpu_attrs_ Maybe enabled_cpu_attrs_? I'm also wondering if we need this additional map - can we just modify cpu_attrs_ for testing purposes? On principle I'd prefer to keep the product code as simple as possible even if it makes the test code a little more complex. http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.h@745 PS2, Line 745: /// Mapping between CpuInfo flags and the corresponding strings. Can you briefly document the key? Since it seems like you're doing something non-trivial with bitwise negation. http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.cc@127 PS2, Line 127: const std::map<int64_t, std::string> LlvmCodeGen::cpu_flag_mappings_{ nit: we generally avoid std:: prefixes in .cc files. common/names.h actually imports a lot of these standard classes into the namespace. http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/util/bit-util-test.cc File be/src/util/bit-util-test.cc: http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/util/bit-util-test.cc@325 PS2, Line 325: const int64_t CPU_INFO_FLAG = CpuInfo::SSSE3; Either const or constexpr here seems fine - why the change? http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/util/cpu-info.cc File be/src/util/cpu-info.cc: http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/util/cpu-info.cc@71 PS2, Line 71: const int64_t CpuInfo::SSSE3 = (1 << 1); We still want to have the constant values visible in the header file so they can be inlined in other modules. You can do that and just define them here without the value, i.e. const int64_t CpuInfo::SSE4_1; Normally the benefit of inlining is minimal, but the CpuInfo checks are done on some very frequently-executed code paths. -- To view, visit http://gerrit.cloudera.org:8080/10979 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f Gerrit-Change-Number: 10979 Gerrit-PatchSet: 2 Gerrit-Owner: Pooja Nilangekar <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Pooja Nilangekar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 19 Jul 2018 00:31:11 +0000 Gerrit-HasComments: Yes
