Copilot commented on code in PR #12894:
URL: https://github.com/apache/gluten/pull/12894#discussion_r3851929883
##########
cpp/CMakeLists.txt:
##########
@@ -179,6 +186,23 @@ endif()
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${SCRIPT_CXX_FLAGS}")
+if(ENABLE_LTO)
+ check_ipo_supported(
+ RESULT GLUTEN_IPO_SUPPORTED
+ OUTPUT GLUTEN_IPO_ERROR
+ LANGUAGES CXX)
+ if(NOT GLUTEN_IPO_SUPPORTED)
+ message(
+ FATAL_ERROR
+ "ENABLE_LTO requested, but IPO/LTO is not supported:
${GLUTEN_IPO_ERROR}"
+ )
+ endif()
+
+ set(CMAKE_INTERPROCEDURAL_OPTIMIZATION ON)
+
+ message(STATUS "ENABLE_LTO enabled for ${CMAKE_BUILD_TYPE} builds")
Review Comment:
The option text says “for release builds”, but
`CMAKE_INTERPROCEDURAL_OPTIMIZATION` is enabled globally (all
configurations/targets). This can unintentionally turn on LTO for Debug builds
and behaves ambiguously with multi-config generators where `CMAKE_BUILD_TYPE`
is empty. Consider enabling IPO per configuration (e.g.,
`CMAKE_INTERPROCEDURAL_OPTIMIZATION_RELEASE` / `..._RELWITHDEBINFO`) or
per-target, and align the status message/option description with the actual
behavior.
##########
cpp/velox/operators/serializer/VeloxRowToColumnarConverter.cc:
##########
@@ -110,8 +110,8 @@ VectorPtr createFlatVector<TypeKind::HUGEINT>(
memcpy(bytesValue.data(), memoryAddress + offsets[pos] + wordoffset,
length);
uint8_t bytesValue2[16]{};
GLUTEN_CHECK(length <= 16, "array out of bounds exception");
- for (int k = length - 1; k >= 0; k--) {
- bytesValue2[length - 1 - k] = bytesValue[k];
+ if (length > 0) {
+ std::reverse_copy(bytesValue.begin(), bytesValue.begin() + length,
bytesValue2);
}
if (static_cast<int8_t>(bytesValue[0]) < 0) {
Review Comment:
`bytesValue[0]` is still accessed even when `length == 0`, which is an
out-of-bounds read. Guard the sign-extension check with `length > 0` (or
provide a safe default path for empty values) so that `bytesValue[0]` is never
read when `length` is zero.
##########
cpp/CMakeLists.txt:
##########
@@ -57,6 +63,7 @@ option(ENABLE_ORC "Enable ORC" OFF)
option(ENABLE_ABFS "Enable ABFS" OFF)
option(ENABLE_GPU "Enable GPU" OFF)
option(ENABLE_ENHANCED_FEATURES "Enable enhanced features" OFF)
+option(ENABLE_LTO "Enable IPO/LTO for release builds" OFF)
Review Comment:
The option text says “for release builds”, but
`CMAKE_INTERPROCEDURAL_OPTIMIZATION` is enabled globally (all
configurations/targets). This can unintentionally turn on LTO for Debug builds
and behaves ambiguously with multi-config generators where `CMAKE_BUILD_TYPE`
is empty. Consider enabling IPO per configuration (e.g.,
`CMAKE_INTERPROCEDURAL_OPTIMIZATION_RELEASE` / `..._RELWITHDEBINFO`) or
per-target, and align the status message/option description with the actual
behavior.
##########
ep/build-velox/src/build-velox.sh:
##########
@@ -139,6 +145,9 @@ function compile {
# INSTALL_PREFIX, producing a version mismatch. BUNDLED skips find_package.
COMPILE_OPTION="$COMPILE_OPTION -Dfmt_SOURCE=BUNDLED"
fi
+ if [ $ENABLE_LTO == "ON" ]; then
+ COMPILE_OPTION="$COMPILE_OPTION -DVELOX_ENABLE_LTO=ON"
+ fi
Review Comment:
The test expression should quote the variable to avoid `[ ... ]` parsing
errors or unintended globbing/word-splitting (e.g., if the value is empty). Use
a quoted comparison (and preferably `=` for POSIX test) to make this robust.
--
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]
