Copilot commented on code in PR #13363:
URL: https://github.com/apache/gluten/pull/13363#discussion_r4344857587
##########
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:
Because `analyze()` is inline in a header, any code outside the `libgluten`
binary that includes the header may call `minMaxScan()` across DSO boundaries.
If the build uses `-fvisibility=hidden` (common for shared libs), the symbol
and/or its ifunc resolver may not be visible, leading to link or runtime
failures. Ensure `minMaxScan` has default visibility (via the project’s export
macro/attribute) in the declaration and definition so external consumers can
resolve the symbol correctly.
##########
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:
Declaring `minMaxScan()` in a header and calling it from an inline
`analyze()` creates a hard link-time dependency on a non-header symbol for
every translation unit that uses `analyze()`. If `ffor.hpp` is consumed outside
the shared library (or the build uses hidden visibility), this can cause
unresolved symbols unless `minMaxScan` is exported. Consider either (mandatory)
marking `minMaxScan` with the project’s export/visibility macro, or (optional)
providing a header-only fallback implementation gated for non-lib builds to
preserve the previous header-only usability.
##########
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) {
+ uint64_t low = data[0];
+ uint64_t high = data[0];
Review Comment:
`minMaxScan()` is now a header-declared utility but relies on the
precondition `n > 0` without any enforcement. Since it’s callable independently
of `analyze()`, it’s easy for future call sites to violate the contract and
trigger out-of-bounds access. Consider adding a cheap debug assertion (e.g.,
project DCHECK/assert) documenting/enforcing `n > 0` at runtime in debug builds.
--
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]
