Copilot commented on code in PR #13363:
URL: https://github.com/apache/gluten/pull/13363#discussion_r4312493060
##########
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
Review Comment:
The PR description says runtime dispatch is enabled for GCC >= 12 or Clang
>= 18, but the implementation explicitly excludes Clang
(`!defined(__clang__)`), meaning Clang builds will always compile the
baseline-only path. Either update the description to match the current GCC-only
implementation, or add a Clang-compatible dispatch mechanism (e.g.,
`__attribute__((target(\"...\")))` variants with an explicit runtime check and
safe gating) so Clang >= 18 also benefits.
##########
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:
`target_clones` relies on toolchain/platform support for multiversion
dispatch (commonly ELF + ifunc). The current guard checks compiler and arch but
not the object format/platform, which can break builds on non-ELF targets where
GCC is present (e.g., some Windows toolchains). Consider tightening the
preprocessor guard (e.g., require `__ELF__` and/or known-supported OS targets)
or adding a feature-test check for `target_clones` support to avoid
compilation/linking failures.
##########
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:
`minMaxScan` is defined in a header as `inline` and uses `target_clones`
(which typically emits an ifunc-style resolver). This can lead to multiple
resolvers/clones being emitted across translation units that include this
header, increasing binary size and potentially adding repeated load-time
resolver overhead. Consider moving `minMaxScan` into a single `.cc`/`.cpp` TU
(declare in the header, define out-of-line) so the resolver/clones are emitted
once per binary.
##########
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:
`analyze()` now always calls the `noinline` `minMaxScan`, adding a
function-call overhead on the smallest inputs (where the call overhead can
dominate). If small `n` is performance-sensitive, add a cheap fast-path in
`analyze()` for small `n` (e.g., inline scalar scan for `n <= 8` or `n <= 16`)
and use `minMaxScan` only for larger blocks where vectorization benefits
amortize the call.
--
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]
