Marcel Kornacker has posted comments on this change. Change subject: IMPALA-2809: Improve ByteSwap with builtin function or SSSE3 or AVX2. ......................................................................
Patch Set 40: (12 comments) http://gerrit.cloudera.org:8080/#/c/3081/40/be/src/util/bit-util-test.cc File be/src/util/bit-util-test.cc: Line 131: void TestByteswapSIMD(const int64_t CpuFlag, const int buf_size) { Simd, not SIMD http://gerrit.cloudera.org:8080/#/c/3081/38/be/src/util/bit-util.cc File be/src/util/bit-util.cc: Line 135: // https://software.intel.com/sites/landingpage/IntrinsicsGuide/ > Hi Marcel. Since this constant is figured out by the definition of _mm_set_ better: "derived from" http://gerrit.cloudera.org:8080/#/c/3081/40/be/src/util/bit-util.cc File be/src/util/bit-util.cc: Line 170: const uint8_t* src = reinterpret_cast<const uint8_t*>(source); if you select simd_func here with an if (template_data_width == 16) simd_func = &byteswap128; ... this should get optimized away (and you can drop that template param) http://gerrit.cloudera.org:8080/#/c/3081/40/be/src/util/bit-util.h File be/src/util/bit-util.h: Line 246: public: formatting (see above for example) Line 247: /// Function ByteSwapScalar is used to byteswap an array in the scalar way with some remove "Function Byte.. is used to " Line 248: /// builtin optimization for the array of length <= 16Byte. 16Byte -> 16 bytes, etc. (and elsewhere) Line 252: static void ByteSwapScalar(void* dst, const void* src, int len); output params go at the end (and elsewhere) Line 262: typedef void (*SimdFuncPointer)(uint8_t* dst, const uint8_t* src); drop "pointer" (we don't tag type names in that way) Line 263: /// Template function ByteSwapSIMD is the entry point function to byteswap an array precede w/ blank line Line 266: /// int TEMPLATE_DATA_WIDTH: only 16 or 32 is availble now; what does "is available now" mean? "are valid"? Line 268: /// if TEMPLATE_DATA_WIDTH is 16, SIMD_FUNC should be ByteSwap128; then why not just call that directly from within the function (and drop the template param)? Line 275: static void ByteSwapSIMD(void* dst, const void* src, const int len); name formatting: Simd, not SIMD -- 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: 40 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Youwei Wang <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Youwei Wang <[email protected]> Gerrit-HasComments: Yes
