Jim Apple has posted comments on this change. Change subject: IMPALA-2809: Improve ByteSwap with builtin function or SSE or AVX2. ......................................................................
Patch Set 17: (8 comments) http://gerrit.cloudera.org:8080/#/c/3081/17/be/src/util/bit-util.inline.h File be/src/util/bit-util.inline.h: Line 25: inline void ByteSwapScalarLoop(void* dst, const void* src, int len) { Could you add a TODO here that this can be made more efficient using BSWAP? Line 136: #ifndef IR_COMPILE I'm not sure we need this. What makes you believe we do need them? Line 144: __attribute__((target("sse4.2"))) I don't think this uses anything beyond SSSE3. Line 156: const __m256i mask256i = _mm256_set_epi8(0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, I think you can make this constexpr and static without the run-time penalty of checking to see if it is initilaized yet by using brace initialization. Line 163: *reinterpret_cast<uint128_t*>(dst + 16) = part1; I think you should vzeroupper at the end; see "Avoiding AVX-SSE Transition Penalties" Line 193: // Remaining bytes(<16) are dealt with scalar routine They might be able to be done faster with pshufb. Add TODO, please Line 200: if ( len >= 32 ) { In the current code, len is always <= 16, right? As such, ByteSwapAVX2_Unit will never be called. I don't think we should add dead code to the repo. One way to avoid this is to make our string reverse function use ByteSwap. Another way is to only do the SSE method for now. Line 217: ByteSwapScalar(dest, source, len); This should be denoted the most LIKELY branch. -- 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: 17 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
