zhaoren...@hotmail.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/15531 )
Change subject: IMPALA-9544 Replace Intel's SSE instructions with ARM's NEON instructions ...................................................................... Patch Set 41: (13 comments) http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/codegen/llvm-codegen-test.cc File be/src/codegen/llvm-codegen-test.cc: http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/codegen/llvm-codegen-test.cc@537 PS40, Line 537: // state. > Can you modify this so that less of this code is within the ifdef. I think Done http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/exec/delimited-text-parser.inline.h File be/src/exec/delimited-text-parser.inline.h: http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/exec/delimited-text-parser.inline.h@242 PS40, Line 242: column_idx_ = num_partition_keys_; > I think you can move this into the #ifndef. Done http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/kudu/util/block_bloom_filter.cc File be/src/kudu/util/block_bloom_filter.cc: PS40: > For all the files under kudu/util, we need to pull in the changes from Kudu Hi, Tim. This file is same with kudu's, I already compare them. http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/util/bit-util-test.cc File be/src/util/bit-util-test.cc: http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/util/bit-util-test.cc@137 PS40, Line 137: int buf_size = 0; > Does this need to be an ifdef? Can we use base::IsAArch64()? I prefer regul Done http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/util/bit-util-test.cc@187 PS40, Line 187: } > Same things here and below - use regular if statements where possible. Done http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/util/bit-util.h File be/src/util/bit-util.h: http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/util/bit-util.h@132 PS40, Line 132: static inline int Popcount(uint64_t x) { > This is really ugly. It would be better just to define a separate version o Done http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/util/bit-util.cc File be/src/util/bit-util.cc: http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/util/bit-util.cc@144 PS40, Line 144: #ifndef __aarch64__ > Can you group together the aarch64 implementations after the x86 implementa Done http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/util/bit-util.cc@196 PS40, Line 196: bswap_fptr = SimdByteSwap::ByteSwap128; > The #ifdefs mixed with control flow is not readable, can you factor out int Done http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/util/bit-util.cc@237 PS40, Line 237: else if (len >= 32) { > Same here, too many #ifdefs mixed in with the control flow. Done http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/util/cpu-info.cc File be/src/util/cpu-info.cc: http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/util/cpu-info.cc@160 PS40, Line 160: if (num_cores > 0) { > This seems confusing. I think we should define a new constant for neon to m Done http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/util/hash-util.h File be/src/util/hash-util.h: http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/util/hash-util.h@42 PS40, Line 42: DCHECK(CpuInfo::IsSupported(CpuInfo::SSE4_2) || base::IsAarch64()); > We need to find a better solution than all these #ifdefs. Done http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/util/sse-util.h File be/src/util/sse-util.h: http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/util/sse-util.h@86 PS40, Line 86: /// support (e.g. IMPALA-6882). > Can you add a comment here like: Done http://gerrit.cloudera.org:8080/#/c/15531/40/be/src/util/sse2neon.h File be/src/util/sse2neon.h: PS40: > Where did this version of the file come from? I would like to know so I can Hi, Tim, the file from here :https://github.com/DLTcollab/sse2neon -- To view, visit http://gerrit.cloudera.org:8080/15531 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id7dfe17125b2910ece54e7dd18b4e4b25d7de8b9 Gerrit-Change-Number: 15531 Gerrit-PatchSet: 41 Gerrit-Owner: Anonymous Coward <zhaoren...@hotmail.com> Gerrit-Reviewer: Anonymous Coward <zhaoren...@hotmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 12 Aug 2020 09:22:08 +0000 Gerrit-HasComments: Yes
