Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15634 )
Change subject: columnar_serialization: use AVX2 for int32 and int64 copying ...................................................................... Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/15634/1/src/kudu/common/columnar_serialization.cc File src/kudu/common/columnar_serialization.cc: http://gerrit.cloudera.org:8080/#/c/15634/1/src/kudu/common/columnar_serialization.cc@323 PS1, Line 323: static bool static const bool http://gerrit.cloudera.org:8080/#/c/15634/1/src/kudu/common/columnar_serialization.cc@328 PS1, Line 328: nt type_size Could you add a comment what's exactly type_size in this context? http://gerrit.cloudera.org:8080/#/c/15634/1/src/kudu/common/columnar_serialization.cc@340 PS1, Line 340: #if __x86_64__ && (defined(__clang__) || (defined(__GNUC__) && __GNUC__ >= 5) Alternatively could use following technique that detects whether compiler supports AVX2 https://github.com/apache/kudu/blob/master/src/kudu/util/CMakeLists.txt#L259 http://gerrit.cloudera.org:8080/#/c/15634/1/src/kudu/common/columnar_serialization.cc@343 PS1, Line 343: 4 For sake of readability, could declare a static constexpr variable size_of_type to be 4/sizeof(int32_t). Same for below. http://gerrit.cloudera.org:8080/#/c/15634/1/src/kudu/common/columnar_serialization.cc@349 PS1, Line 349: int iters = n_sel_rows / 8; It'd be good to have a variable that derives 8 which is basically the number of sel_rows processed in single iteration below. Same for below. http://gerrit.cloudera.org:8080/#/c/15634/1/src/kudu/common/columnar_serialization.cc@351 PS1, Line 351: __m256i indexes = _mm256_cvtepu16_epi32(*reinterpret_cast<const __m128i*>(sel_rows)); Why not load 16 indexes in 256-bit variable instead of 8? http://gerrit.cloudera.org:8080/#/c/15634/1/src/kudu/common/columnar_serialization.cc@349 PS1, Line 349: int iters = n_sel_rows / 8; : while (iters--) { : __m256i indexes = _mm256_cvtepu16_epi32(*reinterpret_cast<const __m128i*>(sel_rows)); : __m256i elems = _mm256_i32gather_epi32(src_buf, indexes, sizeof(int32_t)); : _mm256_storeu_si256(reinterpret_cast<__m256i*>(dst_buf), elems); : dst_buf += 8 * sizeof(int32_t); : sel_rows += 8; : } > I found this difficult to grok without looking at Intel docs. Mind adding a +1. -- To view, visit http://gerrit.cloudera.org:8080/15634 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6c9a536b78a524e8178f5d4a0d2dea04deedbd78 Gerrit-Change-Number: 15634 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 02 Apr 2020 01:52:35 +0000 Gerrit-HasComments: Yes
