Youwei Wang has posted comments on this change. Change subject: IMPALA-2809: Improve ByteSwap with builtin function or SSE or AVX2. ......................................................................
Patch Set 10: (8 comments) http://gerrit.cloudera.org:8080/#/c/3081/10/be/src/benchmarks/bswap-benchmark.cc File be/src/benchmarks/bswap-benchmark.cc: Line 32: // This benchmark is to compare the performance for all available byteswap approaches > Where did the results table go? Done Line 33: // using different data width. Examples used here are: Decimal4, Decimal8 and Decimal6. > Decimal16 Done Line 36: // 2. FasterByteswap: the improved scalar byte-swap; > Improved by whom, and when? Is this the version in the code now? Please giv Done. Line 36: // 2. FasterByteswap: the improved scalar byte-swap; > It might be better to name the benchmarks by their strategy (how they do th Done Line 156: /// Implementation of AVX2 Byteswap using different data width > Ehat does it mean to implement byteswap with different data widths? Done Line 157: template <int num_bytes, int fixed_len_size> > This is still a lot of code duplication. Please try to make it more concise Hi Jim. As for the switch..case statement, I am afraid there exists no too much we can do further. If you take a closer look at these cases, you may notice they are actually different though they look similar. Such tiny differences prevent being extracted to be some common code snippets shared among different cases. It is okay for I will check whether there are some other code sections I can deal with. Thank you. Line 157: template <int num_bytes, int fixed_len_size> > Please make template parameters formatting follow our style guide. Done Line 187: 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 12, 13, 8, 9, 4, 5, 0, 1, > Can you explain where these constants come from? These constants comes from the definition of the bit-mask for the _mm256_shuffle_epi8 intrinsic. Please refer this link if you are interested in more details: https://software.intel.com/sites/landingpage/IntrinsicsGuide/#text=_mm256_shuffle_epi8&expand=4619,3130,4699 In conclusion, we can get these constants backward since we have known the input and can calculate the final output easily. -- 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: 10 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
