Youwei Wang has posted comments on this change. Change subject: IMPALA-2809: Improve ByteSwap with builtin function or SSSE3 or AVX2. ......................................................................
Patch Set 32: (9 comments) http://gerrit.cloudera.org:8080/#/c/3081/32/be/src/benchmarks/bswap-benchmark.cc File be/src/benchmarks/bswap-benchmark.cc: PS32, Line 1: 2015 > 2016 Done Line 46: /// OldImpala 1.224e+06 1X > Can you rebase and rerun? There is a new benchmarking output system Hi Jim. I have rebased this patch. And there arises another issue: I use make_debug.sh/make_release.sh to build Impala. The make_debug.sh works perfectly. However, make_release.sh will output the following errors: CMakeFiles/bit-util-test.dir/bit-util-test.cc.o: In function `impala::TestByteswapSimd(long, int)': /root/Impala/be/src/util/bit-util-test.cc:151: undefined reference to `void impala::SimdByteSwap::ByteSwapSIMD<32, &impala::SimdByteSwap::ByteSwapAVX2_Unit>(void*, void const*, int)' /root/Impala/be/src/util/bit-util-test.cc:148: undefined reference to `void impala::SimdByteSwap::ByteSwapSIMD<16, &impala::SimdByteSwap::ByteSwapSSE_Unit>(void*, void const*, int)' collect2: error: ld returned 1 exit status After diff these two scripts, I found: diff /root/Impala/bin/make_release.sh /root/Impala/bin/make_debug.sh < $IMPALA_HOME/bin/make_impala.sh -build_type=Release $* --- > $IMPALA_HOME/bin/make_impala.sh -build_type=Debug -build_shared_libs $* Then I tried: $IMPALA_HOME/bin/make_impala.sh -build_type=Release -build_shared_libs. I still got the above errors. If you don't mind, would you please share me some clues of how to solve this? Thanks. :) PS32, Line 62: case 1: : *reinterpret_cast<int8_t*>(dst) = *reinterpret_cast<const int8_t*>(src); : return; : case 2: : *reinterpret_cast<int16_t*>(dst) = : BitUtil::ByteSwap(*reinterpret_cast<const int16_t*>(src)); : return; : case 4: : *reinterpret_cast<int32_t*>(dst) = : BitUtil::ByteSwap(*reinterpret_cast<const int32_t*>(src)); : return; : case 8: : *reinterpret_cast<int64_t*>(dst) = : BitUtil::ByteSwap(*reinterpret_cast<const int64_t*>(src)); : return; > These all look like dead code to me, which means this doesn't actually call Done Line 90: static inline void Verification() { > Please give this function a comment Done Line 92: uint8_t* SourceData = new uint8_t[DataLen]; > Don't use new explicitly if you don't have to. Try std::vector Hi Jim. The std::vector is not applicable here since the input data should be in the form of unsigned byte array. So in this case, do you think using C-style malloc will be more praticable here? PS32, Line 118: Scala > "Scalar", not "Scala", here and elsewhere Done Line 145: data.inbuffer = new uint8_t[datalen]; > Again, please avoid new. Hi Jim. The std::vector is not applicable here since the input data should be in the form of unsigned byte array. So in this case, do you think using C-style malloc will be more praticable here? http://gerrit.cloudera.org:8080/#/c/3081/32/be/src/util/bit-util.cc File be/src/util/bit-util.cc: Line 153: uint128_t part1 = *reinterpret_cast<uint128_t*>(dst); > why not use __m128i? Done http://gerrit.cloudera.org:8080/#/c/3081/32/be/src/util/bit-util.h File be/src/util/bit-util.h: Line 250: void ByteSwapScalarLoop(void* dst, const void* src, int len); > Is this used outside of this header? If not, please put in only in the .cc Hi Jim. These declarations will be referred by bswap-benchmark.cc, bit-util.cc and bit-util-test.cc. So in this case, I think maybe it will be better to put such declarations in this header file. Thanks. :) -- To view, visit http://gerrit.cloudera.org:8080/3081 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I392ed5a8d5683f30f161282c228c1aedd7b648c1 Gerrit-PatchSet: 32 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Youwei Wang <youwei.a.w...@intel.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbap...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Mostafa Mokhtar <mmokh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Youwei Wang <youwei.a.w...@intel.com> Gerrit-HasComments: Yes
