Youwei Wang has posted comments on this change. Change subject: IMPALA-2809: Improve ByteSwap with builtin function or SSSE3 or AVX2. ......................................................................
Patch Set 29: (5 comments) http://gerrit.cloudera.org:8080/#/c/3081/28/be/src/util/bit-util-test.cc File be/src/util/bit-util-test.cc: Line 95: void TestByteswapSimdUnit(const int64_t CpuFlag) { > Please explain all function parameters. Done Line 101: FuncPointer BytewapFunc = NULL; > If you only ever test the SSSE3 function with 16 and the AVX2 functino with Done Line 119: EXPECT_EQ(dst_buf[j], src_buf[buf_size - j - 1]); > Please give this function a comment, and please explain all parameters. Done Line 169: EXPECT_EQ(BitUtil::ByteSwap(static_cast<int32_t>(0)), 0); > If the meachine that this tst is running on supports AVX2 and SSSE3, please Done PS28, Line 180: Swap(static_cas > This doesn't test "comprehensively", it just tests the non-SIMD version. Hi Jim. I am sorry but I am afraid this is not that case. If we set the parameter CpuFlag == 0, this code will be executed: BitUtil::ByteSwap(dst_buf, src_buf, i); According its implementation in bit-util.inline.h: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> else if (len >= 32) { // AVX2 can only be used to process data whose size >= 32byte if (CpuInfo::IsSupported(CpuInfo::AVX2)) { ByteSwapSIMD<32, ByteSwapAVX2_Unit>(dest, source, len); } else if (LIKELY(CpuInfo::IsSupported(CpuInfo::SSSE3))) { // SSSE3 support is more popular than AVX2. ByteSwapSIMD<16, ByteSwapSSE_Unit>(dest, source, len); } else { ByteSwapScalar(dest, source, len); } <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< This code ByteSwapSIMD<32, ByteSwapAVX2_Unit>(dest, source, len); will be executed on my workstation. And it varies according actual hardware capability. So this is the season why I added the adverb "comprehensively" there. And I have modified this comment to be: // Test BitUtil::ByteSwap(Black Box Testing) Do you think this new comment is more practicable? Thank you for any idea or suggestion. :) -- 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: 29 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
