Copilot commented on code in PR #13363:
URL: https://github.com/apache/gluten/pull/13363#discussion_r4344720873
##########
cpp/core/utils/tac/ffor.hpp:
##########
@@ -280,23 +280,22 @@ inline void decodeRt(const uint64_t* in, uint64_t* out,
uint64_t base, size_t n,
detail::kDecodeTable[bw](in, out, base, n);
}
+namespace detail {
+// Minimum and maximum of data[0, n); requires n > 0. Defined in
FForCodec.cc, where it is
+// multiversioned for x86-64-v4 (AVX2 has no unsigned 64-bit min/max).
+void minMaxScan(const uint64_t* data, size_t n, uint64_t& mn, uint64_t& mx);
Review Comment:
This change makes `analyze()` (an inline header function) depend on an
out-of-line symbol (`detail::minMaxScan`) defined in `FForCodec.cc`. That
introduces a new link-time dependency for any target that includes `ffor.hpp`
and calls `analyze()`, and can break header-only or partial-link consumers that
previously didn’t need to link `FForCodec.cc`. Consider keeping a header-local
scalar fallback (e.g., under a compile-time macro) or providing an inline
wrapper that falls back when multiversioning isn’t available, so `analyze()`
remains usable without requiring that TU in every link.
##########
cpp/core/utils/tac/ffor.hpp:
##########
@@ -280,23 +280,22 @@ inline void decodeRt(const uint64_t* in, uint64_t* out,
uint64_t base, size_t n,
detail::kDecodeTable[bw](in, out, base, n);
}
+namespace detail {
+// Minimum and maximum of data[0, n); requires n > 0. Defined in
FForCodec.cc, where it is
+// multiversioned for x86-64-v4 (AVX2 has no unsigned 64-bit min/max).
+void minMaxScan(const uint64_t* data, size_t n, uint64_t& mn, uint64_t& mx);
+} // namespace detail
Review Comment:
The declaration opens a plain `namespace detail` at the current scope, while
the definition is nested under `gluten::ffor::detail` in `FForCodec.cc`. This
works only if `ffor.hpp` is guaranteed to be in the same enclosing namespaces
at this point. To make the linkage robust and self-evident, declare
`minMaxScan` in the explicit full namespace that matches the definition (or
place it inside the same enclosing `namespace gluten { namespace ffor { ... }
}` block in the header).
##########
cpp/core/utils/tac/ffor.hpp:
##########
@@ -280,23 +280,22 @@ inline void decodeRt(const uint64_t* in, uint64_t* out,
uint64_t base, size_t n,
detail::kDecodeTable[bw](in, out, base, n);
}
+namespace detail {
+// Minimum and maximum of data[0, n); requires n > 0. Defined in
FForCodec.cc, where it is
+// multiversioned for x86-64-v4 (AVX2 has no unsigned 64-bit min/max).
+void minMaxScan(const uint64_t* data, size_t n, uint64_t& mn, uint64_t& mx);
+} // namespace detail
+
// Compute base (min) and bitwidth for a vector of values.
inline void analyze(const uint64_t* data, size_t n, uint64_t& base, unsigned&
bw) {
if (n == 0) {
base = 0;
bw = 0;
return;
}
- uint64_t mn = data[0];
- uint64_t mx = data[0];
- for (size_t i = 1; i < n; ++i) {
- if (data[i] < mn) {
- mn = data[i];
- }
- if (data[i] > mx) {
- mx = data[i];
- }
- }
+ uint64_t mn = 0;
+ uint64_t mx = 0;
+ detail::minMaxScan(data, n, mn, mx);
Review Comment:
This change makes `analyze()` (an inline header function) depend on an
out-of-line symbol (`detail::minMaxScan`) defined in `FForCodec.cc`. That
introduces a new link-time dependency for any target that includes `ffor.hpp`
and calls `analyze()`, and can break header-only or partial-link consumers that
previously didn’t need to link `FForCodec.cc`. Consider keeping a header-local
scalar fallback (e.g., under a compile-time macro) or providing an inline
wrapper that falls back when multiversioning isn’t available, so `analyze()`
remains usable without requiring that TU in every link.
##########
cpp/core/utils/tac/FForCodec.cc:
##########
@@ -19,6 +19,36 @@
#include "utils/tac/ffor.hpp"
namespace gluten {
+namespace ffor {
+namespace detail {
+
+// AVX2 has no unsigned 64-bit min/max (vpminuq/vpmaxuq need AVX-512), so the
baseline build emulates
+// them with a bias, a signed compare and a blend on the loop-carried chain.
target_clones adds an
+// x86-64-v4 version and an ifunc that picks one at load time; the result is
identical.
Review Comment:
The comment describes a bias/compare/blend sequence, but the function body
is a scalar loop; any bias/compare/blend would come from compiler
autovectorization rather than being implemented here. Consider rewording to
explicitly attribute the emulation sequence to the compiler’s AVX2
vectorization of this loop, to avoid confusion for readers looking for that
logic in the source.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
