hhr293 commented on code in PR #13363:
URL: https://github.com/apache/gluten/pull/13363#discussion_r4344917118
##########
cpp/core/utils/tac/ffor.hpp:
##########
@@ -280,23 +280,55 @@ inline void decodeRt(const uint64_t* in, uint64_t* out,
uint64_t base, size_t n,
detail::kDecodeTable[bw](in, out, base, n);
}
+// AVX2 has no unsigned 64-bit min/max -- vpminuq and vpmaxuq need AVX-512 --
so at the build
+// baseline gcc emulates each comparison with a sign-bit bias, a signed
compare and a variable blend,
+// and the blend lands on the loop-carried accumulator chain. target_clones
has the compiler emit one
+// version of this loop per target, plus an ifunc resolver that picks between
them once at load time:
+// the default clone keeps that emulation, the x86-64-v4 clone collapses it to
one instruction per
+// accumulator. Results are identical either way; this is the same unsigned
min/max.
+//
+// Kept out of analyze() and noinline on purpose: inlined, gcc 13 gives up
vectorizing this loop, and
+// writing through mn / mx inside it makes gcc assume they alias data and
refuse to vectorize at all.
+//
+// Multiversioning is on from GCC 12, the first release that can resolve the
level at runtime: GCC 11
+// accepts -march=x86-64-v4 but has no __builtin_cpu_supports for the level,
so it rejects the
+// attribute outright instead of ignoring it. Clang is excluded because it
rejects target_clones
+// together with noinline. Those builds, and non-x86-64 targets, compile a
plain baseline function.
+#ifndef GLUTEN_ENABLE_MULTITARGET_CODE
+#define GLUTEN_ENABLE_MULTITARGET_CODE 1
+#endif
+#if GLUTEN_ENABLE_MULTITARGET_CODE && defined(__x86_64__) && defined(__GNUC__)
&& !defined(__clang__) && __GNUC__ >= 12
Review Comment:
Addressed: the guard also requires `__ELF__` now (see the `#if` in
`FForCodec.cc`), so non-ELF targets compile the plain function.
##########
cpp/core/utils/tac/ffor.hpp:
##########
@@ -280,23 +280,55 @@ inline void decodeRt(const uint64_t* in, uint64_t* out,
uint64_t base, size_t n,
detail::kDecodeTable[bw](in, out, base, n);
}
+// AVX2 has no unsigned 64-bit min/max -- vpminuq and vpmaxuq need AVX-512 --
so at the build
+// baseline gcc emulates each comparison with a sign-bit bias, a signed
compare and a variable blend,
+// and the blend lands on the loop-carried accumulator chain. target_clones
has the compiler emit one
+// version of this loop per target, plus an ifunc resolver that picks between
them once at load time:
+// the default clone keeps that emulation, the x86-64-v4 clone collapses it to
one instruction per
+// accumulator. Results are identical either way; this is the same unsigned
min/max.
+//
+// Kept out of analyze() and noinline on purpose: inlined, gcc 13 gives up
vectorizing this loop, and
+// writing through mn / mx inside it makes gcc assume they alias data and
refuse to vectorize at all.
+//
+// Multiversioning is on from GCC 12, the first release that can resolve the
level at runtime: GCC 11
+// accepts -march=x86-64-v4 but has no __builtin_cpu_supports for the level,
so it rejects the
+// attribute outright instead of ignoring it. Clang is excluded because it
rejects target_clones
+// together with noinline. Those builds, and non-x86-64 targets, compile a
plain baseline function.
+#ifndef GLUTEN_ENABLE_MULTITARGET_CODE
+#define GLUTEN_ENABLE_MULTITARGET_CODE 1
+#endif
+#if GLUTEN_ENABLE_MULTITARGET_CODE && defined(__x86_64__) && defined(__GNUC__)
&& !defined(__clang__) && __GNUC__ >= 12
+#define GLUTEN_FFOR_MULTIVERSION
__attribute__((target_clones("arch=x86-64-v4", "default")))
+#else
+#define GLUTEN_FFOR_MULTIVERSION
+#endif
+
+GLUTEN_FFOR_MULTIVERSION __attribute__((noinline)) inline void
Review Comment:
Addressed: the definition moved to `FForCodec.cc` and `ffor.hpp` only
declares `detail::minMaxScan`. The clones and the resolver are emitted once per
library, and only one TU defines the function, so there is no per-TU macro/ODR
issue.
##########
cpp/core/utils/tac/ffor.hpp:
##########
@@ -280,23 +280,55 @@ inline void decodeRt(const uint64_t* in, uint64_t* out,
uint64_t base, size_t n,
detail::kDecodeTable[bw](in, out, base, n);
}
+// AVX2 has no unsigned 64-bit min/max -- vpminuq and vpmaxuq need AVX-512 --
so at the build
+// baseline gcc emulates each comparison with a sign-bit bias, a signed
compare and a variable blend,
+// and the blend lands on the loop-carried accumulator chain. target_clones
has the compiler emit one
+// version of this loop per target, plus an ifunc resolver that picks between
them once at load time:
+// the default clone keeps that emulation, the x86-64-v4 clone collapses it to
one instruction per
+// accumulator. Results are identical either way; this is the same unsigned
min/max.
+//
+// Kept out of analyze() and noinline on purpose: inlined, gcc 13 gives up
vectorizing this loop, and
+// writing through mn / mx inside it makes gcc assume they alias data and
refuse to vectorize at all.
+//
+// Multiversioning is on from GCC 12, the first release that can resolve the
level at runtime: GCC 11
+// accepts -march=x86-64-v4 but has no __builtin_cpu_supports for the level,
so it rejects the
+// attribute outright instead of ignoring it. Clang is excluded because it
rejects target_clones
+// together with noinline. Those builds, and non-x86-64 targets, compile a
plain baseline function.
+#ifndef GLUTEN_ENABLE_MULTITARGET_CODE
+#define GLUTEN_ENABLE_MULTITARGET_CODE 1
+#endif
+#if GLUTEN_ENABLE_MULTITARGET_CODE && defined(__x86_64__) && defined(__GNUC__)
&& !defined(__clang__) && __GNUC__ >= 12
+#define GLUTEN_FFOR_MULTIVERSION
__attribute__((target_clones("arch=x86-64-v4", "default")))
+#else
+#define GLUTEN_FFOR_MULTIVERSION
+#endif
+
+GLUTEN_FFOR_MULTIVERSION __attribute__((noinline)) inline void
+minMaxScan(const uint64_t* data, size_t n, uint64_t& mn, uint64_t& mx) {
+ uint64_t low = data[0];
+ uint64_t high = data[0];
+ for (size_t i = 1; i < n; ++i) {
+ if (data[i] < low) {
+ low = data[i];
+ }
+ if (data[i] > high) {
+ high = data[i];
+ }
+ }
+ mn = low;
+ mx = high;
+}
+
// 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;
+ minMaxScan(data, n, mn, mx);
Review Comment:
Measured, and not kept: an inline scalar path for blocks of 16 values or
fewer removed most of the short-block cost, but it also gives up the AVX-512
gain at those sizes and changes nothing from 33 values up. With the AVX2
default clone, short blocks are slower (+12% at 4 values, up to +16% at 8), but
`compress64Impl` emits at most one such trailing block per call and the default
shuffle compression threshold is 100 rows. From 100 values up the default clone
is within -1% to +4% of the previous code.
##########
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:
Intentional: defining the multiversioned function in the header made every
including TU emit its own clones and resolver (see the inline thread). Only
`FForCodec.cc` and `FForCodecTest.cc` (which links `libgluten`) include
`ffor.hpp`, and the declaration comment now says callers must link
`FForCodec.cc`. Symbol visibility is tracked in the separate thread below.
##########
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 is inside `namespace gluten { namespace ffor {` (opened near
the top of `ffor.hpp` and closed at the end), so it is
`gluten::ffor::detail::minMaxScan`, matching the definition in `FForCodec.cc`.
It links in the unit test and in `libgluten`.
--
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]
