Copilot commented on code in PR #49940:
URL: https://github.com/apache/arrow/pull/49940#discussion_r3349013509
##########
cpp/src/arrow/util/cpu_info.h:
##########
@@ -70,16 +66,19 @@ class ARROW_EXPORT CpuInfo {
static const CpuInfo* GetInstance();
/// Returns all the flags for this cpu
- int64_t hardware_flags() const;
+ int64_t hardware_flags() const { return hardware_flags_; }
/// Returns the number of cores (including hyper-threaded) on this machine.
- int num_cores() const;
+ int num_cores() const { return num_cores_ <= 0 ? 1 : num_cores_; }
/// Returns the vendor of the cpu.
- Vendor vendor() const;
+ Vendor vendor() const { return vendor_; }
/// Returns the model name of the cpu (e.g. Intel i7-2600)
- const std::string& model_name() const;
+ std::string_view model_name() const {
+ // Unavailable in xsimd at the time of migration and previously unused.
+ return "Unknown";
+ }
Review Comment:
This changes the public API/ABI of `CpuInfo::model_name()` from `const
std::string&` to `std::string_view`, and also changes behavior to always return
a literal. Even in `arrow::internal`, `ARROW_EXPORT` suggests this can be
consumed across shared library boundaries. Consider preserving the existing
signature/ABI (e.g., keep returning `const std::string&` backed by a member) or
introducing a new accessor while keeping the old one for compatibility, and
update the comment/doc accordingly.
##########
cpp/src/arrow/util/cpu_info.cc:
##########
@@ -17,64 +17,107 @@
// From Apache Impala (incubating) as of 2016-01-29.
-#include "arrow/util/cpu_info.h"
-
-#ifdef __APPLE__
-# include <sys/sysctl.h>
-#endif
-
-#ifndef _MSC_VER
-# include <unistd.h>
-#endif
-
-#ifdef _WIN32
-# include <intrin.h>
-
-# include "arrow/util/windows_compatibility.h"
-#endif
-
#include <algorithm>
#include <array>
-#include <bitset>
#include <cctype>
#include <cerrno>
#include <cstdint>
-#include <fstream>
-#include <memory>
#include <optional>
#include <string>
#include <thread>
+#include <xsimd/xsimd.hpp>
+
#include "arrow/result.h"
+#include "arrow/util/cpu_info.h"
#include "arrow/util/io_util.h"
#include "arrow/util/logging_internal.h"
#include "arrow/util/string.h"
-#undef CPUINFO_ARCH_X86
-#undef CPUINFO_ARCH_ARM
-#undef CPUINFO_ARCH_PPC
-
-#if defined(__i386) || defined(_M_IX86) || defined(__x86_64__) ||
defined(_M_X64)
-# define CPUINFO_ARCH_X86
-#elif defined(_M_ARM64) || defined(__aarch64__) || defined(__arm64__)
-# define CPUINFO_ARCH_ARM
-#elif defined(__PPC64__) || defined(__PPC64LE__) || defined(__ppc64__) || \
- defined(__powerpc64__)
-# define CPUINFO_ARCH_PPC
+#ifdef __linux__
+# include <fstream>
#endif
-namespace arrow {
-namespace internal {
+#ifdef __APPLE__
+# include <sys/sysctl.h>
+#endif
+
+#ifndef _MSC_VER
+# include <unistd.h>
+#endif
+
+#ifdef _WIN32
+# include <intrin.h>
+
+# include "arrow/util/windows_compatibility.h"
+#endif
+
+namespace arrow::internal {
namespace {
-constexpr int kCacheLevels = static_cast<int>(CpuInfo::CacheLevel::Last) + 1;
+void OsRetrieveCpuInfo(int64_t* hardware_flags, CpuInfo::Vendor* vendor) {
+ const auto cpu = xsimd::cpu_features();
+
+ *hardware_flags |= cpu.popcnt() ? CpuInfo::POPCNT : 0;
+ *hardware_flags |= cpu.bmi1() ? CpuInfo::BMI1 : 0;
+ *hardware_flags |= cpu.bmi2() ? CpuInfo::BMI2 : 0;
+
+ // SSE
+ *hardware_flags |= cpu.ssse3() ? CpuInfo::SSSE3 : 0;
+ *hardware_flags |= cpu.sse4_1() ? CpuInfo::SSE4_1 : 0;
+ *hardware_flags |= cpu.sse4_2() ? CpuInfo::SSE4_2 : 0;
+ // AVX
+ *hardware_flags |= cpu.avx() ? CpuInfo::AVX : 0;
+ *hardware_flags |= cpu.avx2() ? CpuInfo::AVX2 : 0;
+ // AVX 512
+ const bool avx512f = cpu.avx512f();
+ *hardware_flags |= cpu.avx512f() ? CpuInfo::AVX512F : 0;
+ *hardware_flags |= cpu.avx512cd() ? CpuInfo::AVX512CD : 0;
+ *hardware_flags |= cpu.avx512dq() ? CpuInfo::AVX512DQ : 0;
+ *hardware_flags |= cpu.avx512bw() ? CpuInfo::AVX512BW : 0;
+ // TODO(xsimd): Missing in xsimd 14.2.0 but fixed afterwards.
+ // Can be replaced with the following (no `if(avx512f)` required).
+ // *hardware_flags |= cpu.avx512vl() ? CpuInfo::AVX512VL : 0;
+ if (avx512f) {
+ const auto cpu_x86 = xsimd::x86_cpu_features_backend_default();
+ auto constexpr avx512vl = static_cast<xsimd::x86_cpuid_leaf7::ebx>(31);
+ *hardware_flags |= cpu_x86.leaf7().all_bits_set<avx512vl>() ?
CpuInfo::AVX512VL : 0;
+ }
Review Comment:
The AVX512VL detection looks like it’s casting the integer `31` as the bit
*mask* enum value, but cpuid feature enums are commonly defined as `(1u <<
bit_index)`. If `xsimd::x86_cpuid_leaf7::ebx` is a mask enum, `31` would check
the wrong bits and mis-detect AVX512VL. Prefer using the named enum constant
(if available) or the correct mask value for bit 31 (e.g., `1u << 31`) per
xsimd’s cpuid API.
##########
cpp/src/arrow/util/cpu_info.cc:
##########
@@ -550,95 +368,64 @@ void ArchVerifyCpuRequirements(const CpuInfo* ci) {}
} // namespace
-struct CpuInfo::Impl {
- int64_t hardware_flags = 0;
- int num_cores = 0;
- int64_t original_hardware_flags = 0;
- Vendor vendor = Vendor::Unknown;
- std::string model_name = "Unknown";
- std::array<int64_t, kCacheLevels> cache_sizes{};
-
- Impl() {
- OsRetrieveCacheSize(&cache_sizes);
- OsRetrieveCpuInfo(&hardware_flags, &vendor, &model_name);
- original_hardware_flags = hardware_flags;
- num_cores =
std::max(static_cast<int>(std::thread::hardware_concurrency()), 1);
-
- // parse user simd level
- auto maybe_env_var = GetEnvVar("ARROW_USER_SIMD_LEVEL");
- if (!maybe_env_var.ok()) {
- return;
- }
- std::string s = *std::move(maybe_env_var);
- std::transform(s.begin(), s.end(), s.begin(),
- [](unsigned char c) { return std::toupper(c); });
- if (!ArchParseUserSimdLevel(s, &hardware_flags)) {
- ARROW_LOG(WARNING) << "Invalid value for ARROW_USER_SIMD_LEVEL: " << s;
- }
- }
+CpuInfo::CpuInfo() {
+ OsRetrieveCacheSize(&cache_sizes_);
+ OsRetrieveCpuInfo(&hardware_flags_, &vendor_);
+ original_hardware_flags_ = hardware_flags_;
+ num_cores_ = std::max(static_cast<int>(std::thread::hardware_concurrency()),
1);
- void EnableFeature(int64_t flag, bool enable) {
- if (!enable) {
- hardware_flags &= ~flag;
- } else {
- // Can't turn something on that can't be supported
- DCHECK_EQ((~original_hardware_flags) & flag, 0);
- hardware_flags |= (flag & original_hardware_flags);
- }
+ // parse user simd level
+ auto maybe_env_var = GetEnvVar("ARROW_USER_SIMD_LEVEL");
+ if (!maybe_env_var.ok()) {
+ return;
}
-};
-
-CpuInfo::~CpuInfo() = default;
-
-CpuInfo::CpuInfo() : impl_(new Impl) {}
+ std::string s = *std::move(maybe_env_var);
+ std::transform(s.begin(), s.end(), s.begin(),
+ [](unsigned char c) { return std::toupper(c); });
+ if (!ArchParseUserSimdLevel(s, &hardware_flags_)) {
+ ARROW_LOG(WARNING) << "Invalid value for ARROW_USER_SIMD_LEVEL: " << s;
+ }
+}
const CpuInfo* CpuInfo::GetInstance() {
- static CpuInfo cpu_info;
+ static const CpuInfo cpu_info;
return &cpu_info;
}
Review Comment:
The singleton is now `static const CpuInfo`, but `CpuInfo` has mutating
methods (notably `EnableFeature`) that update `hardware_flags_`. If any
existing code uses `EnableFeature` via the singleton (even via `const_cast`),
modifying a `const` object is undefined behavior. Consider making the singleton
non-const again (`static CpuInfo cpu_info;`) or removing/making immutable any
mutating public APIs if the intent is to enforce constness.
--
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]
