hhr293 commented on code in PR #13363:
URL: https://github.com/apache/gluten/pull/13363#discussion_r4346355921
##########
cpp/core/utils/tac/FForCodec.cc:
##########
@@ -19,6 +19,37 @@
#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) {
Review Comment:
Addressed in f0e7ab2: `minMaxScan` is now declared with
`__attribute__((visibility("default")))`, and a visibility attribute on the
first declaration applies to the definition. Checked by building `FForCodec.cc`
with `-fvisibility=hidden` and the libgluten version script, then linking an
external caller of `analyze()`: with `target_clones` (GCC 13.4) the IFUNC and
its resolver are exported; with `GLUTEN_ENABLE_MULTITARGET_CODE=0` the plain
function is exported, and without the attribute that case fails to link.
##########
cpp/core/utils/tac/ffor.hpp:
##########
@@ -280,23 +280,23 @@ 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).
+void minMaxScan(const uint64_t* data, size_t n, uint64_t& mn, uint64_t& mx);
Review Comment:
By design. Having `analyze()` call an out-of-line symbol is the point of
this layout: a multiversioned function defined in the header makes every
including TU emit its own clones and resolver (see the earlier thread). In the
repo `ffor.hpp` is only included by `FForCodec.cc` (compiled into
`gluten_core_impl`, hence `libgluten`) and by `FForCodecTest.cc`, which links
`gluten_core_impl`, so there is no consumer that lacks the symbol. The export
side is handled by the default-visibility attribute (f0e7ab2), and the
declaration comment documents that callers must link `FForCodec.cc`. A
header-only fallback is not added.
##########
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:
Keeping this as is. `minMaxScan` is a `detail::` helper whose only caller is
`analyze()`, and `analyze()` returns early for `n == 0` before calling it, so
the precondition holds on every call path. An early return inside the scan
would add a check to every call for a case that cannot occur through the public
entry point, and would need an arbitrary choice of `mn`/`mx` for empty input.
The precondition is documented at the declaration and checked in debug builds
by `GLUTEN_DCHECK`.
--
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]
