Copilot commented on code in PR #13363:
URL: https://github.com/apache/gluten/pull/13363#discussion_r4345997601
##########
cpp/core/utils/tac/ffor.hpp:
##########
@@ -280,23 +280,24 @@ 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).
Callers of analyze() must
+// therefore link FForCodec.cc (part of libgluten). Default visibility keeps
the symbol exported
+// from libgluten under -fvisibility=hidden when target_clones is compiled out
(plain function).
+__attribute__((visibility("default"))) 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:
`analyze()` is an inline header function, but it now requires an out-of-line
definition of `detail::minMaxScan()` from `FForCodec.cc`. This turns a
previously header-only operation into one that will fail to link for any
consumer that includes `ffor.hpp` without also linking the library/object that
provides `minMaxScan` (e.g., unit tests/tools that used to rely only on
headers). If `ffor.hpp` is intended to be usable independently, consider
providing a header inline fallback implementation when the library symbol isn’t
available, or move `analyze()` out-of-line alongside `minMaxScan()` to make the
dependency explicit.
##########
cpp/core/utils/tac/FForCodec.cc:
##########
@@ -16,9 +16,42 @@
*/
#include "utils/tac/FForCodec.h"
+#include "utils/Exception.h"
#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
GCC's AVX2 vectorization of
+// this loop emulates them with a bias, a signed compare and a blend on the
loop-carried chain. The
+// source is a plain scalar loop; target_clones adds an x86-64-v4 version and
an ifunc that picks
+// one at load time, and the result is identical.
+// Defined here so the clones and the resolver are emitted once, not once per
including TU.
+// GCC 11 rejects the attribute (no runtime x86-64-v4 level check) and clang's
support is untested,
+// so those builds, non-ELF/non-x86-64 targets and
GLUTEN_ENABLE_MULTITARGET_CODE=0 get a plain function.
+#if defined(__GNUC__) && !defined(__clang__) && __GNUC__ >= 12 &&
defined(__x86_64__) && defined(__ELF__) && \
+ (!defined(GLUTEN_ENABLE_MULTITARGET_CODE) ||
GLUTEN_ENABLE_MULTITARGET_CODE)
+__attribute__((target_clones("arch=x86-64-v4", "default")))
+#endif
+void minMaxScan(const uint64_t* data, size_t n, uint64_t& mn, uint64_t& mx) {
+ GLUTEN_DCHECK(n > 0, "minMaxScan requires at least one value");
Review Comment:
`minMaxScan()` dereferences `data[0]` unconditionally, but the `n > 0` guard
is only a `GLUTEN_DCHECK`. If `GLUTEN_DCHECK` compiles out in release builds, a
caller passing `n == 0` will hit UB. Prefer a non-DCHECK enforcement (e.g., a
release check) or handle `n == 0` with an early return that initializes `mn/mx`
safely.
##########
cpp/core/utils/tac/ffor.hpp:
##########
@@ -280,23 +280,24 @@ 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).
Callers of analyze() must
+// therefore link FForCodec.cc (part of libgluten). Default visibility keeps
the symbol exported
+// from libgluten under -fvisibility=hidden when target_clones is compiled out
(plain function).
+__attribute__((visibility("default"))) void minMaxScan(const uint64_t* data,
size_t n, uint64_t& mn, uint64_t& mx);
Review Comment:
The unconditional use of `__attribute__((visibility("default")))` in a
header is a GNU/Clang extension and can break builds on non-GNU compilers
(notably MSVC). Consider guarding this attribute with compiler checks or using
the project’s existing export/visibility macro so the header remains portable
across supported toolchains.
--
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]
