Youwei Wang has posted comments on this change. Change subject: IMPALA-2809: improve ByteSwap with builtin function or SSE or AVX2 ......................................................................
Patch Set 5: (4 comments) http://gerrit.cloudera.org:8080/#/c/3081/5/be/src/benchmarks/bswap-benchmark.cc File be/src/benchmarks/bswap-benchmark.cc: Line 36: // Impala: the old Impala implementation. > I'm sorry, but this is still confusing. What does "old" mean, here? Is Bswa Old Impala implementation refers to some code has been discarded in previous patch. I reserve a copy as the function ImpalaByteSwap in this source file. I will improve such description as your request. http://gerrit.cloudera.org:8080/#/c/3081/5/be/src/util/bit-util.inline.h File be/src/util/bit-util.inline.h: Line 161: template <int template_data_width> > template params should be in caps, like a constant. We follow https://googl Done Line 167: if(len >= 16) data_width = 16; > Please put both branches of a conditional inside {} Done Line 174: static const FuncPointer SIMDFuncTable[2] = {ByteSwapSSE_Unit, ByteSwapAVX2_Unit}; > I think you can make this an actual function parameter, which might compile Hi Jim. If you don't mind, I just want to make sure that you suggest to make the variable func_idx an actual function parameter? If so, I am afraid this is not the case since the func_idx is used as an index to choose the correct function from the jump table "SIMDFuncTable". Maybe we can still make it as an actual function parameter but it requires to unify the ByteSwapSSE_Unit and ByteSwapSSE_Unit functions. I am afraid this is not practical according to our context. -- 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: 5 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
