westonpace commented on code in PR #36662:
URL: https://github.com/apache/arrow/pull/36662#discussion_r1262844641
##########
cpp/src/arrow/acero/bloom_filter_test.cc:
##########
@@ -324,9 +329,13 @@ void TestBloomLargeHashHelper(int64_t hardware_flags,
int64_t block,
// Test with larger size Bloom filters (use large prime with arithmetic
// sequence modulo 2^64).
//
-void TestBloomLarge(BloomFilterBuildStrategy strategy, int64_t num_build, bool
use_simd,
+void TestBloomLarge(BloomFilterBuildStrategy strategy, int64_t num_build, bool
use_avx2,
bool enable_prefetch) {
- int64_t hardware_flags = use_simd ? ::arrow::internal::CpuInfo::AVX2 : 0;
+ int64_t hardware_flags = use_avx2 ? CpuInfo::AVX2 : 0;
+ if (hardware_flags && !CpuInfo::GetInstance()->IsSupported(hardware_flags)) {
+ // What else?
Review Comment:
Same as above.
##########
cpp/src/arrow/acero/bloom_filter_test.cc:
##########
@@ -171,8 +172,12 @@ void TestBloomSmallHashHelper(int64_t num_input_hashes,
const T* input_hashes,
// Output FPR and build and probe cost.
//
void TestBloomSmall(BloomFilterBuildStrategy strategy, int64_t num_build,
- int num_build_copies, bool use_simd, bool enable_prefetch)
{
- int64_t hardware_flags = use_simd ? ::arrow::internal::CpuInfo::AVX2 : 0;
+ int num_build_copies, bool use_avx2, bool enable_prefetch)
{
+ int64_t hardware_flags = use_avx2 ? CpuInfo::AVX2 : 0;
+ if (hardware_flags && !CpuInfo::GetInstance()->IsSupported(hardware_flags)) {
+ // What else?
+ return;
Review Comment:
This is a parameterized test. So either we need to change the
parameterization in `TEST(BloomFilter, Basic)` to never set `use_avx2` to true
if it isn't supported (in which case we can remove this if check) or we can
just do `GTEST_SKIP() << "AVX2 not supported on CPU"`.
##########
cpp/src/arrow/compute/key_hash_test.cc:
##########
@@ -140,14 +141,19 @@ class TestVectorHash {
hashes_simd32.resize(num_rows);
hashes_simd64.resize(num_rows);
- int64_t hardware_flags_scalar = 0LL;
- int64_t hardware_flags_simd = ::arrow::internal::CpuInfo::AVX2;
+ const int64_t hardware_flags_scalar = 0LL;
+ const int64_t hardware_flags_simd = CpuInfo::AVX2;
+ const bool simd_supported =
CpuInfo::GetInstance()->IsSupported(hardware_flags_simd);
constexpr int mini_batch_size = 1024;
std::vector<uint32_t> temp_buffer;
temp_buffer.resize(mini_batch_size * 4);
for (bool use_simd : {false, true}) {
+ if (use_simd && !simd_supported) {
+ // XXX what else?
+ continue;
+ }
Review Comment:
```suggestion
std::vector<bool> use_simd_opts = simd_supported ? {false, true} :
{false};
for (bool use_simd : use_simd_opts) {
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
