You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/11/18 09:52:49 UTC

[GitHub] [arrow] zhoudan-intel opened a new pull request, #14674: [C++] Accelerate Parquet bit-packing decoding with ICX AVX-512 instru…

zhoudan-intel opened a new pull request, #14674:
URL: https://github.com/apache/arrow/pull/14674

   …ctions
   
   Per the basic test (BM_DictDecodingInt64_literals of the parquet-encoding-benchmark test suite) on Intel ICX platform, up to 55% performance improvement is observed.
   
   Authored-by: Zhou, Dan <da...@intel.com>
   Co-authored-by: Wan, Weiqiang <we...@intel.com>
   Co-authored-by: Zhao, Yaqi <ya...@intel.com>
   Signed-off-by: Zhou, Dan <da...@intel.com>


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kou commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
kou commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1043824240


##########
cpp/src/arrow/util/cpu_info.cc:
##########
@@ -363,6 +363,10 @@ int64_t LinuxParseCpuFlags(const std::string& values) {
     {"avx512bw", CpuInfo::AVX512BW},
     {"bmi1", CpuInfo::BMI1},
     {"bmi2", CpuInfo::BMI2},
+    {"avx512ifma", CpuInfo::AVX512IFMA},
+    {"avx512vbmi", CpuInfo::AVX512VBMI},
+    {"avx512_vbmi2", CpuInfo::AVX512VBMI2},
+    {"avx512_vnni", CpuInfo::AVX512VNNI},

Review Comment:
   ```suggestion
       {"avx512vbmi2", CpuInfo::AVX512VBMI2},
       {"avx512vnni", CpuInfo::AVX512VNNI},
   ```



##########
cpp/src/arrow/util/cpu_info.h:
##########
@@ -53,7 +53,11 @@ class ARROW_EXPORT CpuInfo {
   static constexpr int64_t AVX512 = AVX512F | AVX512CD | AVX512VL | AVX512DQ | AVX512BW;
   static constexpr int64_t BMI1 = (1LL << 11);
   static constexpr int64_t BMI2 = (1LL << 12);
-
+  static constexpr int64_t AVX512IFMA = (1LL << 13);
+  static constexpr int64_t AVX512VBMI = (1LL << 14);
+  static constexpr int64_t AVX512VBMI2 = (1LL << 15);
+  static constexpr int64_t AVX512VNNI = (1LL << 16);
+  static constexpr int64_t AVX512_ICX = AVX512IFMA | AVX512VBMI | AVX512VNNI;

Review Comment:
   Should this also include `AVX512VBMI2`?



##########
cpp/src/arrow/util/bpacking.cc:
##########
@@ -29,6 +29,9 @@
 #if defined(ARROW_HAVE_RUNTIME_AVX512)
 #include "arrow/util/bpacking_avx512.h"
 #endif
+#if defined(ARROW_HAVE_AVX512_ICX)

Review Comment:
   ```suggestion
   #if defined(ARROW_HAVE_RUNTIME_AVX512_ICX)
   ```
   
   We need to define `ARROW_HAVE_RUNTIME_AVX512_ICX` in `cpp/cmake_modules/SetupCxxFlags.cmake`.



##########
cpp/src/arrow/util/bpacking.cc:
##########
@@ -166,14 +169,34 @@ struct Unpack32DynamicFunction {
   }
 };
 
+#if defined(ARROW_HAVE_AVX512_ICX)
+struct Unpack32DynamicFunctionICX {
+  using FunctionType = decltype(&unpack32_default);
+
+  static std::vector<std::pair<DispatchLevel, FunctionType>> implementations() {
+    return {{DispatchLevel::NONE, unpack32_default},
+            {DispatchLevel::AVX512_ICX, unpack32_avx512_icx}};
+  }
+};

Review Comment:
   We can't reuse `Unpack32DynamicFunction` for `AVX512_ICX` because `unpack32_avx512_icx()` doesn't support `num_bits > 16`, right?
   Can we use `unpack32_specialized()` for `num_bits > 16` like other `unpack32_*()`?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] amol- closed pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by "amol- (via GitHub)" <gi...@apache.org>.
amol- closed pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions
URL: https://github.com/apache/arrow/pull/14674


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
zhoudan-intel commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1033289205


##########
cpp/src/arrow/util/bpacking_avx512_icx_inline.h:
##########
@@ -0,0 +1,1139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include "bpacking_avx512_icx.h"
+#include "arrow/util/logging.h"
+
+#include <algorithm>
+#include <type_traits>
+#include <limits.h>
+#include <immintrin.h>
+
+#include <boost/preprocessor/repetition/repeat_from_to.hpp>

Review Comment:
   We've removed the boost function.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #14674:
URL: https://github.com/apache/arrow/pull/14674#issuecomment-1370070030

   I'm quite lukewarm about this. We should ensure the code can easily be maintained and we also need to be sure it is correct. Since it's not obvious how it was written/generated nor what the algorithm is, none of those two conditions are satisfied.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] github-actions[bot] commented on pull request #14674: [C++] Accelerate Parquet bit-packing decoding with ICX AVX-512 instru…

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14674:
URL: https://github.com/apache/arrow/pull/14674#issuecomment-1319775809

   <!--
     Licensed to the Apache Software Foundation (ASF) under one
     or more contributor license agreements.  See the NOTICE file
     distributed with this work for additional information
     regarding copyright ownership.  The ASF licenses this file
     to you under the Apache License, Version 2.0 (the
     "License"); you may not use this file except in compliance
     with the License.  You may obtain a copy of the License at
   
       http://www.apache.org/licenses/LICENSE-2.0
   
     Unless required by applicable law or agreed to in writing,
     software distributed under the License is distributed on an
     "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
     KIND, either express or implied.  See the License for the
     specific language governing permissions and limitations
     under the License.
   -->
   
   Thanks for opening a pull request!
   
   If this is not a [minor PR](https://github.com/apache/arrow/blob/master/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW
   
   Opening JIRAs ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
   
   Then could you also rename pull request title in the following format?
   
       ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}
   
   or
   
       MINOR: [${COMPONENT}] ${SUMMARY}
   
   See also:
   
     * [Other pull requests](https://github.com/apache/arrow/pulls/)
     * [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1060818654


##########
cpp/cmake_modules/SetupCxxFlags.cmake:
##########
@@ -82,22 +90,45 @@ if(ARROW_CPU_FLAG STREQUAL "x86")
         return 0;
       }"
                               CXX_SUPPORTS_AVX512)
+
+    set(CMAKE_REQUIRED_FLAGS ${OLD_CMAKE_REQURED_FLAGS})
+
+    set(OLD_CMAKE_REQURED_FLAGS ${CMAKE_REQUIRED_FLAGS})

Review Comment:
   This is pointless.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] wanweiqiangintel commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
wanweiqiangintel commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1049622617


##########
cpp/src/arrow/util/bpacking.cc:
##########
@@ -166,14 +169,34 @@ struct Unpack32DynamicFunction {
   }
 };
 
+#if defined(ARROW_HAVE_AVX512_ICX)
+struct Unpack32DynamicFunctionICX {
+  using FunctionType = decltype(&unpack32_default);
+
+  static std::vector<std::pair<DispatchLevel, FunctionType>> implementations() {
+    return {{DispatchLevel::NONE, unpack32_default},
+            {DispatchLevel::AVX512_ICX, unpack32_avx512_icx}};
+  }
+};

Review Comment:
   Hi, @kou , yes, unpack32_avx512_icx() doesn't support num_bits > 16 currently, but the support of num_bits between 17 to 32 is in progress and almost done. We need to do more tests about this and if it goes well, we will upstream the implementation.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by "zhoudan-intel (via GitHub)" <gi...@apache.org>.
zhoudan-intel commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1100991838


##########
cpp/src/arrow/util/bpacking_avx512_icx.h:
##########
@@ -0,0 +1,1145 @@
+// Licensed to the Apache Software Foundation (ASF) under one

Review Comment:
   @pitrou Hey, thanks for your question. This code is written by hand now. I agree that AVX512 intrinsic is not obvious and not easy to understand, but it shows great performance improvement. For this case up to 55% performance improvement is observed on the Intel ICX platform. Regarding the maintainance issue, we can verify its function via unit tests, function tests, etc. If more functions/features are introduced in the future, we will add more test cases to ensure it works as designed. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kou commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
kou commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1026822634


##########
cpp/src/arrow/util/bpacking_avx512_icx_inline.h:
##########
@@ -0,0 +1,1139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include "bpacking_avx512_icx.h"
+#include "arrow/util/logging.h"
+
+#include <algorithm>
+#include <type_traits>
+#include <limits.h>
+#include <immintrin.h>
+
+#include <boost/preprocessor/repetition/repeat_from_to.hpp>
+
+#define ALWAYS_INLINE __attribute__((always_inline))

Review Comment:
   `__attribute__((always_inline))` doesn't work with MSVC.
   `__forceinline` is the syntax for it in MSVC: https://learn.microsoft.com/en-us/cpp/cpp/inline-functions-cpp?view=msvc-170
   
   We don't want to define a macro without prefix in public headers.
   
   BTW, why do we need this?



##########
cpp/src/arrow/util/bpacking_avx512_icx.h:
##########
@@ -0,0 +1,227 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstddef>
+#include <cstdint>
+#include <utility>
+#include <immintrin.h>
+
+namespace arrow {
+namespace internal {
+
+/// Utilities for manipulating bit-packed values. Bit-packing is a technique for
+/// compressing integer values that do not use the full range of the integer type.
+/// E.g. an array of uint32_t values with range [0, 31] only uses the lower 5 bits
+/// of every uint32_t value, or an array of 0/1 booleans only uses the lowest bit
+/// of each integer.
+///
+/// Bit-packing always has a "bit width" parameter that determines the range of
+/// representable unsigned values: [0, 2^bit_width - 1]. The packed representation
+/// is logically the concatenatation of the lower bits of the input values (in
+/// little-endian order). E.g. the values 1, 2, 3, 4 packed with bit width 4 results
+/// in the two output bytes: [ 0 0 1 0 | 0 0 0 1 ] [ 0 1 0 0 | 0 0 1 1 ]
+///                               2         1           4         3
+///
+/// Packed values can be split across words, e.g. packing 1, 17 with bit_width 5 results
+/// in the two output bytes: [ 0 0 1 | 0 0 0 0 1 ] [ x x x x x x | 1 0 ]
+///            lower bits of 17--^         1         next value     ^--upper bits of 17
+///
+/// Bit widths from 0 to 64 are supported (0 bit width means that every value is 0).
+/// The batched unpacking functions operate on batches of 32 values. This batch size
+/// is convenient because for every supported bit width, the end of a 32 value batch
+/// falls on a byte boundary. It is also large enough to amortise loop overheads.
+class BitPacking {

Review Comment:
   It seems that all methods are defined as `static`. (We don't instantiate this.)
   How about using `namespace` instead?



##########
cpp/src/arrow/util/bpacking_avx512_icx_inline.h:
##########
@@ -0,0 +1,1139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include "bpacking_avx512_icx.h"
+#include "arrow/util/logging.h"
+
+#include <algorithm>
+#include <type_traits>
+#include <limits.h>
+#include <immintrin.h>
+
+#include <boost/preprocessor/repetition/repeat_from_to.hpp>

Review Comment:
   We don't want to use Boost for public header to avoid build-time Boost dependency.



##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -340,17 +344,32 @@ inline int BitReader::GetBatch(int num_bits, T* v, int batch_size) {
     }
   }
 
+  int num_unpacked = 0;
   if (sizeof(T) == 4) {
-    int num_unpacked =

Review Comment:
   Could you revert this scope change to keep narrower variable scope?



##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -25,6 +25,10 @@
 #include <cstdint>
 
 #include "arrow/util/bit_util.h"
+#if defined(ARROW_HAVE_RUNTIME_AVX512_ICX)
+#include "arrow/util/bpacking_avx512_icx.h"
+#include "arrow/util/bpacking_avx512_icx_inline.h"

Review Comment:
   Why should we split these header files?



##########
cpp/src/arrow/util/bpacking_avx512_icx_inline.h:
##########
@@ -0,0 +1,1139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include "bpacking_avx512_icx.h"
+#include "arrow/util/logging.h"
+
+#include <algorithm>
+#include <type_traits>
+#include <limits.h>
+#include <immintrin.h>
+
+#include <boost/preprocessor/repetition/repeat_from_to.hpp>
+
+#define ALWAYS_INLINE __attribute__((always_inline))
+#define UNLIKELY(expr) __builtin_expect(!!(expr), 0)
+#define LIKELY(expr) __builtin_expect(!!(expr), 1)

Review Comment:
   Can we use existing `ARROW_PREDICT_FALSE()`/`ARROW_PREDICT_TRUE()` instead?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] amol- commented on pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by "amol- (via GitHub)" <gi...@apache.org>.
amol- commented on PR #14674:
URL: https://github.com/apache/arrow/pull/14674#issuecomment-1490645684

   Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
zhoudan-intel commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1029417289


##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -340,17 +344,32 @@ inline int BitReader::GetBatch(int num_bits, T* v, int batch_size) {
     }
   }
 
+  int num_unpacked = 0;
   if (sizeof(T) == 4) {
-    int num_unpacked =

Review Comment:
   Sure. I reverted the scope change.



##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -340,17 +344,32 @@ inline int BitReader::GetBatch(int num_bits, T* v, int batch_size) {
     }
   }
 
+  int num_unpacked = 0;
   if (sizeof(T) == 4) {
-    int num_unpacked =

Review Comment:
   I reverted the scope change.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] github-actions[bot] commented on pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14674:
URL: https://github.com/apache/arrow/pull/14674#issuecomment-1320443316

   https://issues.apache.org/jira/browse/ARROW-18362


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
zhoudan-intel commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1029417289


##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -340,17 +344,32 @@ inline int BitReader::GetBatch(int num_bits, T* v, int batch_size) {
     }
   }
 
+  int num_unpacked = 0;
   if (sizeof(T) == 4) {
-    int num_unpacked =

Review Comment:
   Sure. I will revert the scope change.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
zhoudan-intel commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1033419864


##########
cpp/src/arrow/util/bpacking_avx512_icx.h:
##########
@@ -0,0 +1,227 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstddef>
+#include <cstdint>
+#include <utility>
+#include <immintrin.h>
+
+namespace arrow {
+namespace internal {
+
+/// Utilities for manipulating bit-packed values. Bit-packing is a technique for
+/// compressing integer values that do not use the full range of the integer type.
+/// E.g. an array of uint32_t values with range [0, 31] only uses the lower 5 bits
+/// of every uint32_t value, or an array of 0/1 booleans only uses the lowest bit
+/// of each integer.
+///
+/// Bit-packing always has a "bit width" parameter that determines the range of
+/// representable unsigned values: [0, 2^bit_width - 1]. The packed representation
+/// is logically the concatenatation of the lower bits of the input values (in
+/// little-endian order). E.g. the values 1, 2, 3, 4 packed with bit width 4 results
+/// in the two output bytes: [ 0 0 1 0 | 0 0 0 1 ] [ 0 1 0 0 | 0 0 1 1 ]
+///                               2         1           4         3
+///
+/// Packed values can be split across words, e.g. packing 1, 17 with bit_width 5 results
+/// in the two output bytes: [ 0 0 1 | 0 0 0 0 1 ] [ x x x x x x | 1 0 ]
+///            lower bits of 17--^         1         next value     ^--upper bits of 17
+///
+/// Bit widths from 0 to 64 are supported (0 bit width means that every value is 0).
+/// The batched unpacking functions operate on batches of 32 values. This batch size
+/// is convenient because for every supported bit width, the end of a 32 value batch
+/// falls on a byte boundary. It is also large enough to amortise loop overheads.
+class BitPacking {

Review Comment:
   The code has been updated per suggestions.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
zhoudan-intel commented on PR #14674:
URL: https://github.com/apache/arrow/pull/14674#issuecomment-1329361665

   @kou The code has been updated per your suggestions. Could you continue your review? Thanks.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
zhoudan-intel commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1057142070


##########
cpp/src/arrow/util/bpacking.cc:
##########
@@ -29,6 +29,9 @@
 #if defined(ARROW_HAVE_RUNTIME_AVX512)
 #include "arrow/util/bpacking_avx512.h"
 #endif
+#if defined(ARROW_HAVE_AVX512_ICX)

Review Comment:
   @kou The code has been changed. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
zhoudan-intel commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1050413441


##########
cpp/src/arrow/util/cpu_info.cc:
##########
@@ -363,6 +363,10 @@ int64_t LinuxParseCpuFlags(const std::string& values) {
     {"avx512bw", CpuInfo::AVX512BW},
     {"bmi1", CpuInfo::BMI1},
     {"bmi2", CpuInfo::BMI2},
+    {"avx512ifma", CpuInfo::AVX512IFMA},
+    {"avx512vbmi", CpuInfo::AVX512VBMI},
+    {"avx512_vbmi2", CpuInfo::AVX512VBMI2},
+    {"avx512_vnni", CpuInfo::AVX512VNNI},

Review Comment:
   @kou Thanks for your suggestions! But I think "avx512_vbmi2"/"avx512_vnni" could not be renamed because they have to be consistent with the value of flags in the /proc/cpuinfo file.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1049623526


##########
cpp/src/arrow/util/bpacking.cc:
##########
@@ -166,14 +169,34 @@ struct Unpack32DynamicFunction {
   }
 };
 
+#if defined(ARROW_HAVE_AVX512_ICX)
+struct Unpack32DynamicFunctionICX {
+  using FunctionType = decltype(&unpack32_default);
+
+  static std::vector<std::pair<DispatchLevel, FunctionType>> implementations() {
+    return {{DispatchLevel::NONE, unpack32_default},
+            {DispatchLevel::AVX512_ICX, unpack32_avx512_icx}};
+  }
+};

Review Comment:
   Or `unpack32_avx512_icx` should simply fallback on `unpack32_avx512` for `num_bits > 16`. We should not have two different dynamic dispatches for this.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #14674:
URL: https://github.com/apache/arrow/pull/14674#issuecomment-1343416648

   Can you explain what "Intel ICX" is? How is it different from regular AVX512?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
zhoudan-intel commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1033418290


##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -25,6 +25,10 @@
 #include <cstdint>
 
 #include "arrow/util/bit_util.h"
+#if defined(ARROW_HAVE_RUNTIME_AVX512_ICX)
+#include "arrow/util/bpacking_avx512_icx.h"
+#include "arrow/util/bpacking_avx512_icx_inline.h"

Review Comment:
   We combined these 2 files into bpacking_avx512_icx.h.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
zhoudan-intel commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1039749939


##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -341,11 +344,27 @@ inline int BitReader::GetBatch(int num_bits, T* v, int batch_size) {
   }
 
   if (sizeof(T) == 4) {
-    int num_unpacked =
-        internal::unpack32(reinterpret_cast<const uint32_t*>(buffer + byte_offset),
-                           reinterpret_cast<uint32_t*>(v + i), batch_size - i, num_bits);
-    i += num_unpacked;
-    byte_offset += num_unpacked * num_bits / 8;
+    if (num_bits <= 16) {
+#if defined(ARROW_HAVE_AVX512_ICX)

Review Comment:
   @wgtmac I agree. It is better to put the logic into the dispatcher instead of the bit_stream_utils.h file. Code has been updated. Please review it. Thanks. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1060821594


##########
cpp/cmake_modules/SetupCxxFlags.cmake:
##########
@@ -82,22 +90,45 @@ if(ARROW_CPU_FLAG STREQUAL "x86")
         return 0;
       }"
                               CXX_SUPPORTS_AVX512)
+
+    set(CMAKE_REQUIRED_FLAGS ${OLD_CMAKE_REQURED_FLAGS})
+
+    set(OLD_CMAKE_REQURED_FLAGS ${CMAKE_REQUIRED_FLAGS})
+    set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} ${ARROW_AVX512_ICX_FLAG}")
+    check_cxx_source_compiles("
+      #ifdef _MSC_VER
+      #include <intrin.h>
+      #else
+      #include <immintrin.h>
+      #endif
+
+      int main() {
+        __m512i data = _mm512_set1_epi32(0x1);
+	_mm512_maskz_expand_epi16(0x1f1f1f1f, data);
+        return 0;
+      }"
+                              CXX_SUPPORTS_AVX512_ICX)
     set(CMAKE_REQUIRED_FLAGS ${OLD_CMAKE_REQURED_FLAGS})
   endif()
   # Runtime SIMD level it can get from compiler and ARROW_RUNTIME_SIMD_LEVEL
   if(CXX_SUPPORTS_SSE4_2 AND ARROW_RUNTIME_SIMD_LEVEL MATCHES
-                             "^(SSE4_2|AVX2|AVX512|MAX)$")
+                             "^(SSE4_2|AVX2|AVX512|AVX512_ICX|MAX)$")
     set(ARROW_HAVE_RUNTIME_SSE4_2 ON)
     add_definitions(-DARROW_HAVE_RUNTIME_SSE4_2)
   endif()
-  if(CXX_SUPPORTS_AVX2 AND ARROW_RUNTIME_SIMD_LEVEL MATCHES "^(AVX2|AVX512|MAX)$")
+  if(CXX_SUPPORTS_AVX2 AND ARROW_RUNTIME_SIMD_LEVEL MATCHES
+                           "^(AVX2|AVX512|AVX512_ICX|MAX)$")
     set(ARROW_HAVE_RUNTIME_AVX2 ON)
     add_definitions(-DARROW_HAVE_RUNTIME_AVX2 -DARROW_HAVE_RUNTIME_BMI2)
   endif()
-  if(CXX_SUPPORTS_AVX512 AND ARROW_RUNTIME_SIMD_LEVEL MATCHES "^(AVX512|MAX)$")
+  if(CXX_SUPPORTS_AVX512 AND ARROW_RUNTIME_SIMD_LEVEL MATCHES "^(AVX512|AVX512_ICX|MAX)$")
     set(ARROW_HAVE_RUNTIME_AVX512 ON)
     add_definitions(-DARROW_HAVE_RUNTIME_AVX512 -DARROW_HAVE_RUNTIME_BMI2)
   endif()
+  if(CXX_SUPPORTS_AVX512_ICX AND ARROW_RUNTIME_SIMD_LEVEL MATCHES "^(AVX512_ICX|MAX)$")
+    set(ARROW_HAVE_RUNTIME_AVX512_ICX ON)
+    add_definitions(-DARROW_HAVE_RUNTIME_AVX512_ICX -DARROW_HAVE_RUNTIME_BMI2)

Review Comment:
   It seems `-DARROW_HAVE_RUNTIME_BMI2` will be added several times (first for AVX2, then for AVX512, then for AVX512_ICX...).



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
zhoudan-intel commented on PR #14674:
URL: https://github.com/apache/arrow/pull/14674#issuecomment-1367395046

   @kou @pitrou Hi, we modified the PR per your review suggestions. Could you please take a look at it? Thank you!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1060822817


##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -246,6 +254,8 @@ endif()
 
 append_avx2_src(util/bpacking_avx2.cc)
 append_avx512_src(util/bpacking_avx512.cc)
+append_avx512_icx_src(util/bpacking_avx512_icx.h)
+append_avx512_icx_src(util/bpacking.cc)

Review Comment:
   Why are you adding `util/bpacking.cc` here? This shouldn't be needed.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1060819352


##########
cpp/cmake_modules/SetupCxxFlags.cmake:
##########
@@ -48,22 +48,30 @@ if(ARROW_CPU_FLAG STREQUAL "x86")
     set(ARROW_SSE4_2_FLAG "")
     set(ARROW_AVX2_FLAG "/arch:AVX2")
     set(ARROW_AVX512_FLAG "/arch:AVX512")
+    set(ARROW_AVX512_ICX_FLAG "/arch:AVX512")
     set(CXX_SUPPORTS_SSE4_2 TRUE)
   else()
     set(ARROW_SSE4_2_FLAG "-msse4.2")
     set(ARROW_AVX2_FLAG "-march=haswell")
     # skylake-avx512 consists of AVX512F,AVX512BW,AVX512VL,AVX512CD,AVX512DQ
     set(ARROW_AVX512_FLAG "-march=skylake-avx512 -mbmi2")
+    # icelake-server consists of AVX512VBMI,AVX512IFMA,AVX512VBMI2,AVX512VPOPCNTDQ
+    #   AVX512BITALG,AVX512VNN
+    set(ARROW_AVX512_ICX_FLAG "-march=icelake-server -mbmi2")
     # Append the avx2/avx512 subset option also, fix issue ARROW-9877 for homebrew-cpp
     set(ARROW_AVX2_FLAG "${ARROW_AVX2_FLAG} -mavx2")
     set(ARROW_AVX512_FLAG
         "${ARROW_AVX512_FLAG} -mavx512f -mavx512cd -mavx512vl -mavx512dq -mavx512bw")
+    set(ARROW_AVX512_ICX_FLAG
+        "${ARROW_AVX512_ICX_FLAG} -mavx512ifma -mavx512vbmi -mavx512vnni -mavx512vbmi2")

Review Comment:
   Don't you need to also append the other AVX512 subsets, such as `-mavx512f -mavx512cd -mavx512vl -mavx512dq -mavx512bw`?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1060820110


##########
cpp/cmake_modules/SetupCxxFlags.cmake:
##########
@@ -82,22 +90,45 @@ if(ARROW_CPU_FLAG STREQUAL "x86")
         return 0;
       }"
                               CXX_SUPPORTS_AVX512)
+
+    set(CMAKE_REQUIRED_FLAGS ${OLD_CMAKE_REQURED_FLAGS})
+
+    set(OLD_CMAKE_REQURED_FLAGS ${CMAKE_REQUIRED_FLAGS})

Review Comment:
   Also, there's a typo... ("OLD_CMAKE_REQURED_FLAGS")



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
zhoudan-intel commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1033419864


##########
cpp/src/arrow/util/bpacking_avx512_icx.h:
##########
@@ -0,0 +1,227 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstddef>
+#include <cstdint>
+#include <utility>
+#include <immintrin.h>
+
+namespace arrow {
+namespace internal {
+
+/// Utilities for manipulating bit-packed values. Bit-packing is a technique for
+/// compressing integer values that do not use the full range of the integer type.
+/// E.g. an array of uint32_t values with range [0, 31] only uses the lower 5 bits
+/// of every uint32_t value, or an array of 0/1 booleans only uses the lowest bit
+/// of each integer.
+///
+/// Bit-packing always has a "bit width" parameter that determines the range of
+/// representable unsigned values: [0, 2^bit_width - 1]. The packed representation
+/// is logically the concatenatation of the lower bits of the input values (in
+/// little-endian order). E.g. the values 1, 2, 3, 4 packed with bit width 4 results
+/// in the two output bytes: [ 0 0 1 0 | 0 0 0 1 ] [ 0 1 0 0 | 0 0 1 1 ]
+///                               2         1           4         3
+///
+/// Packed values can be split across words, e.g. packing 1, 17 with bit_width 5 results
+/// in the two output bytes: [ 0 0 1 | 0 0 0 0 1 ] [ x x x x x x | 1 0 ]
+///            lower bits of 17--^         1         next value     ^--upper bits of 17
+///
+/// Bit widths from 0 to 64 are supported (0 bit width means that every value is 0).
+/// The batched unpacking functions operate on batches of 32 values. This batch size
+/// is convenient because for every supported bit width, the end of a 32 value batch
+/// falls on a byte boundary. It is also large enough to amortise loop overheads.
+class BitPacking {

Review Comment:
   The code has been updated per suggestions.



##########
cpp/src/arrow/util/bpacking_avx512_icx_inline.h:
##########
@@ -0,0 +1,1139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include "bpacking_avx512_icx.h"
+#include "arrow/util/logging.h"
+
+#include <algorithm>
+#include <type_traits>
+#include <limits.h>
+#include <immintrin.h>
+
+#include <boost/preprocessor/repetition/repeat_from_to.hpp>
+
+#define ALWAYS_INLINE __attribute__((always_inline))
+#define UNLIKELY(expr) __builtin_expect(!!(expr), 0)
+#define LIKELY(expr) __builtin_expect(!!(expr), 1)

Review Comment:
   The code has been updated per suggestions.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #14674:
URL: https://github.com/apache/arrow/pull/14674#issuecomment-1353035591

   @wanweiqiangintel Ok, thanks. So this is not a ICX-specific extension as it also exists on more recent Intel 
   client CPUs, as well as on the most recent AMD CPUs.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
zhoudan-intel commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1057140805


##########
cpp/src/arrow/util/bpacking.cc:
##########
@@ -166,14 +169,34 @@ struct Unpack32DynamicFunction {
   }
 };
 
+#if defined(ARROW_HAVE_AVX512_ICX)
+struct Unpack32DynamicFunctionICX {
+  using FunctionType = decltype(&unpack32_default);
+
+  static std::vector<std::pair<DispatchLevel, FunctionType>> implementations() {
+    return {{DispatchLevel::NONE, unpack32_default},
+            {DispatchLevel::AVX512_ICX, unpack32_avx512_icx}};
+  }
+};

Review Comment:
   @pitrou Hi, we have changed the code per your suggestions. Please review.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] wgtmac commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
wgtmac commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1037891456


##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -341,11 +344,27 @@ inline int BitReader::GetBatch(int num_bits, T* v, int batch_size) {
   }
 
   if (sizeof(T) == 4) {
-    int num_unpacked =
-        internal::unpack32(reinterpret_cast<const uint32_t*>(buffer + byte_offset),
-                           reinterpret_cast<uint32_t*>(v + i), batch_size - i, num_bits);
-    i += num_unpacked;
-    byte_offset += num_unpacked * num_bits / 8;
+    if (num_bits <= 16) {
+#if defined(ARROW_HAVE_AVX512_ICX)

Review Comment:
   I am afraid that more if/else will be added here to support more instruction sets, which makes it less maintainable. Is it possible to move the dispatch logic into `internal::unpack32` implementation and let the dispatcher do the job? In this way, we can avoid exposing `arrow/util/bpacking_avx512_icx.h` in this public header as well.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
zhoudan-intel commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1033289205


##########
cpp/src/arrow/util/bpacking_avx512_icx_inline.h:
##########
@@ -0,0 +1,1139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include "bpacking_avx512_icx.h"
+#include "arrow/util/logging.h"
+
+#include <algorithm>
+#include <type_traits>
+#include <limits.h>
+#include <immintrin.h>
+
+#include <boost/preprocessor/repetition/repeat_from_to.hpp>

Review Comment:
   OK. We remove the boost function.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kou commented on pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
kou commented on PR #14674:
URL: https://github.com/apache/arrow/pull/14674#issuecomment-1329770934

   There is still a lint CI failure: https://github.com/apache/arrow/actions/runs/3563281428/jobs/5997220530
   
   You can fix it by running code formatter: https://arrow.apache.org/docs/developers/cpp/development.html#code-style-linting-and-ci
   
   Could you enable GitHub Actions on your fork? Then, you can confirm CI result on your fork.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
zhoudan-intel commented on PR #14674:
URL: https://github.com/apache/arrow/pull/14674#issuecomment-1342309582

   @wgtmac Thank you for the updates! Sure. Please invite any other reviewers if they are interested in it.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
zhoudan-intel commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1050427786


##########
cpp/src/arrow/util/cpu_info.h:
##########
@@ -53,7 +53,11 @@ class ARROW_EXPORT CpuInfo {
   static constexpr int64_t AVX512 = AVX512F | AVX512CD | AVX512VL | AVX512DQ | AVX512BW;
   static constexpr int64_t BMI1 = (1LL << 11);
   static constexpr int64_t BMI2 = (1LL << 12);
-
+  static constexpr int64_t AVX512IFMA = (1LL << 13);
+  static constexpr int64_t AVX512VBMI = (1LL << 14);
+  static constexpr int64_t AVX512VBMI2 = (1LL << 15);
+  static constexpr int64_t AVX512VNNI = (1LL << 16);
+  static constexpr int64_t AVX512_ICX = AVX512IFMA | AVX512VBMI | AVX512VNNI;

Review Comment:
   It should be. I've updated the code.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1060818164


##########
cpp/cmake_modules/SetupCxxFlags.cmake:
##########
@@ -48,22 +48,30 @@ if(ARROW_CPU_FLAG STREQUAL "x86")
     set(ARROW_SSE4_2_FLAG "")
     set(ARROW_AVX2_FLAG "/arch:AVX2")
     set(ARROW_AVX512_FLAG "/arch:AVX512")
+    set(ARROW_AVX512_ICX_FLAG "/arch:AVX512")
     set(CXX_SUPPORTS_SSE4_2 TRUE)
   else()
     set(ARROW_SSE4_2_FLAG "-msse4.2")
     set(ARROW_AVX2_FLAG "-march=haswell")
     # skylake-avx512 consists of AVX512F,AVX512BW,AVX512VL,AVX512CD,AVX512DQ
     set(ARROW_AVX512_FLAG "-march=skylake-avx512 -mbmi2")
+    # icelake-server consists of AVX512VBMI,AVX512IFMA,AVX512VBMI2,AVX512VPOPCNTDQ
+    #   AVX512BITALG,AVX512VNN
+    set(ARROW_AVX512_ICX_FLAG "-march=icelake-server -mbmi2")
     # Append the avx2/avx512 subset option also, fix issue ARROW-9877 for homebrew-cpp
     set(ARROW_AVX2_FLAG "${ARROW_AVX2_FLAG} -mavx2")
     set(ARROW_AVX512_FLAG
         "${ARROW_AVX512_FLAG} -mavx512f -mavx512cd -mavx512vl -mavx512dq -mavx512bw")
+    set(ARROW_AVX512_ICX_FLAG
+        "${ARROW_AVX512_ICX_FLAG} -mavx512ifma -mavx512vbmi -mavx512vnni -mavx512vbmi2")
     check_cxx_compiler_flag(${ARROW_SSE4_2_FLAG} CXX_SUPPORTS_SSE4_2)
   endif()
   check_cxx_compiler_flag(${ARROW_AVX2_FLAG} CXX_SUPPORTS_AVX2)
   if(MINGW)
     # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65782
     message(STATUS "Disable AVX512 support on MINGW for now")
+  elseif(MSVC)
+    set(CXX_SUPPORTS_AVX512_ICX FALSE)

Review Comment:
   Why this? This disables AVX512 on MSVC as well...



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1060819352


##########
cpp/cmake_modules/SetupCxxFlags.cmake:
##########
@@ -48,22 +48,30 @@ if(ARROW_CPU_FLAG STREQUAL "x86")
     set(ARROW_SSE4_2_FLAG "")
     set(ARROW_AVX2_FLAG "/arch:AVX2")
     set(ARROW_AVX512_FLAG "/arch:AVX512")
+    set(ARROW_AVX512_ICX_FLAG "/arch:AVX512")
     set(CXX_SUPPORTS_SSE4_2 TRUE)
   else()
     set(ARROW_SSE4_2_FLAG "-msse4.2")
     set(ARROW_AVX2_FLAG "-march=haswell")
     # skylake-avx512 consists of AVX512F,AVX512BW,AVX512VL,AVX512CD,AVX512DQ
     set(ARROW_AVX512_FLAG "-march=skylake-avx512 -mbmi2")
+    # icelake-server consists of AVX512VBMI,AVX512IFMA,AVX512VBMI2,AVX512VPOPCNTDQ
+    #   AVX512BITALG,AVX512VNN
+    set(ARROW_AVX512_ICX_FLAG "-march=icelake-server -mbmi2")
     # Append the avx2/avx512 subset option also, fix issue ARROW-9877 for homebrew-cpp
     set(ARROW_AVX2_FLAG "${ARROW_AVX2_FLAG} -mavx2")
     set(ARROW_AVX512_FLAG
         "${ARROW_AVX512_FLAG} -mavx512f -mavx512cd -mavx512vl -mavx512dq -mavx512bw")
+    set(ARROW_AVX512_ICX_FLAG
+        "${ARROW_AVX512_ICX_FLAG} -mavx512ifma -mavx512vbmi -mavx512vnni -mavx512vbmi2")

Review Comment:
   Don't you need to also the other subsets as well, such as `-mavx512f -mavx512cd -mavx512vl -mavx512dq -mavx512bw`?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by "zhoudan-intel (via GitHub)" <gi...@apache.org>.
zhoudan-intel commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1100991838


##########
cpp/src/arrow/util/bpacking_avx512_icx.h:
##########
@@ -0,0 +1,1145 @@
+// Licensed to the Apache Software Foundation (ASF) under one

Review Comment:
   @pitrou Hey, thanks for your question. This code is written by hand now. I agree that AVX512 intrinsic is not obvious and not easy to understand. But it shows great performance improvement. For this case up to 55% performance improvement is observed on the Intel ICX platform. Regarding the maintainance issue, we can verify its function via unit tests, function tests, etc. If more functions/features are introduced in the future, we will add more test cases to ensure it works as designed. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] github-actions[bot] commented on pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14674:
URL: https://github.com/apache/arrow/pull/14674#issuecomment-1320443341

   :warning: Ticket **has not been started in JIRA**, please click 'Start Progress'.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
zhoudan-intel commented on PR #14674:
URL: https://github.com/apache/arrow/pull/14674#issuecomment-1330627994

   @kou Thank you for the detailed information! I've updated the code to resolve lint issues.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
zhoudan-intel commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1033419114


##########
cpp/src/arrow/util/bpacking_avx512_icx_inline.h:
##########
@@ -0,0 +1,1139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include "bpacking_avx512_icx.h"
+#include "arrow/util/logging.h"
+
+#include <algorithm>
+#include <type_traits>
+#include <limits.h>
+#include <immintrin.h>
+
+#include <boost/preprocessor/repetition/repeat_from_to.hpp>
+
+#define ALWAYS_INLINE __attribute__((always_inline))
+#define UNLIKELY(expr) __builtin_expect(!!(expr), 0)
+#define LIKELY(expr) __builtin_expect(!!(expr), 1)

Review Comment:
   The code has been updated per suggestions.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
zhoudan-intel commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1033738009


##########
cpp/src/arrow/util/bpacking_avx512_icx_inline.h:
##########
@@ -0,0 +1,1139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include "bpacking_avx512_icx.h"
+#include "arrow/util/logging.h"
+
+#include <algorithm>
+#include <type_traits>
+#include <limits.h>
+#include <immintrin.h>
+
+#include <boost/preprocessor/repetition/repeat_from_to.hpp>
+
+#define ALWAYS_INLINE __attribute__((always_inline))

Review Comment:
   The code has been updated per suggestions.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zhoudan-intel commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
zhoudan-intel commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1033289205


##########
cpp/src/arrow/util/bpacking_avx512_icx_inline.h:
##########
@@ -0,0 +1,1139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include "bpacking_avx512_icx.h"
+#include "arrow/util/logging.h"
+
+#include <algorithm>
+#include <type_traits>
+#include <limits.h>
+#include <immintrin.h>
+
+#include <boost/preprocessor/repetition/repeat_from_to.hpp>

Review Comment:
   Got it. We've removed the boost function.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1060826458


##########
cpp/src/arrow/util/bpacking_avx512_icx.h:
##########
@@ -0,0 +1,1145 @@
+// Licensed to the Apache Software Foundation (ASF) under one

Review Comment:
   How was this code written? By hand? Was it generated? How is it supposed to be maintained?
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on a diff in pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14674:
URL: https://github.com/apache/arrow/pull/14674#discussion_r1060823821


##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -246,6 +254,8 @@ endif()
 
 append_avx2_src(util/bpacking_avx2.cc)
 append_avx512_src(util/bpacking_avx512.cc)
+append_avx512_icx_src(util/bpacking_avx512_icx.h)

Review Comment:
   I would have expected a `.cc` file.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] wanweiqiangintel commented on pull request #14674: ARROW-18362: [C++][Parquet] Accelerate Parquet bit-packing decoding with ICX AVX-512 instructions

Posted by GitBox <gi...@apache.org>.
wanweiqiangintel commented on PR #14674:
URL: https://github.com/apache/arrow/pull/14674#issuecomment-1353024803

   > 
   
   Hi, @pitrou ,thank you for reviewing this PR. The AVX-512 instruction set consists of several separate sets each having their own unique CPUID feature bit.  Intel ICX(ICE LAKE， 3rd generation Xeon Scalable server processors) introduced the AVX512VBMI2 for the first time.([https://en.wikipedia.org/wiki/AVX-512](url))


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org