Todd Lipcon 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: (13 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 has_avx2 = base::CPU().has_avx2(); > warning: 'has_avx2' is a static definition in anonymous namespace; static i Done http://gerrit.cloudera.org:8080/#/c/15634/1/src/kudu/common/columnar_serialization.cc@323 PS1, Line 323: static bool > static const bool Done http://gerrit.cloudera.org:8080/#/c/15634/1/src/kudu/common/columnar_serialization.cc@328 PS1, Line 328: type_size > nit: rename this sizeof_type too? Done 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? Done (added at the main entry point) http://gerrit.cloudera.org:8080/#/c/15634/1/src/kudu/common/columnar_serialization.cc@330 PS1, Line 330: const uint16_t* __restrict__ sel_rows, > warning: parameter 'sel_rows' is unused [misc-unused-parameters] Done http://gerrit.cloudera.org:8080/#/c/15634/1/src/kudu/common/columnar_serialization.cc@331 PS1, Line 331: int n_sel_rows, > warning: parameter 'n_sel_rows' is unused [misc-unused-parameters] Done http://gerrit.cloudera.org:8080/#/c/15634/1/src/kudu/common/columnar_serialization.cc@332 PS1, Line 332: const uint8_t* __restrict__ src_buf, > warning: parameter 'src_buf' is unused [misc-unused-parameters] Done http://gerrit.cloudera.org:8080/#/c/15634/1/src/kudu/common/columnar_serialization.cc@333 PS1, Line 333: uint8_t* __restrict__ dst_buf) { > warning: parameter 'dst_buf' is unused [misc-unused-parameters] Done 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 s GCC4 supports AVX2 but doesn't know how to expose AVX2 intrinsics on a per-function basis according to the function's target attribute 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_ Done 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 numbe Done 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? we can only fit 8 ints into the vector on the next line, so loading 16 indexes doesn't help us anything. Also the indexes have to be extended to 32-bit because the gather instruction doesn't support 16-bit integers. 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; : } > +1. Done -- 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 <t...@apache.org> Gerrit-Reviewer: Andrew Wong <andrew.w...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Thu, 02 Apr 2020 20:17:46 +0000 Gerrit-HasComments: Yes
