Skip to content

[VL] Support LTO in builds - #12894

Open
zhztheplayer wants to merge 2 commits into
apache:mainfrom
zhztheplayer:wip-lto
Open

[VL] Support LTO in builds#12894
zhztheplayer wants to merge 2 commits into
apache:mainfrom
zhztheplayer:wip-lto

Conversation

@zhztheplayer

Copy link
Copy Markdown
Member

Adds option ENABLE_LTO in build to enable link-time optimization using CMake defaults.

Fixes a bug in code for LTO build.

TEST: enable lto by default

redirect velox

lto
Copilot AI lite review requested due to automatic review settings August 25, 2026 09:55
@zhztheplayer zhztheplayer changed the title [VL] Support LTO in build [VL] Support LTO in builds Aug 25, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Adds a build-time ENABLE_LTO toggle to enable IPO/LTO in the CMake build, and adjusts a HUGEINT byte-reversal path to avoid an invalid loop in LTO builds.

Changes:

  • Introduce --enable_lto / ENABLE_LTO plumbing in Velox and backend build scripts.
  • Add CMake IPO/LTO enablement guarded by an ENABLE_LTO option with capability checks.
  • Replace manual byte reversal with std::reverse_copy in HUGEINT conversion logic.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
ep/build-velox/src/build-velox.sh Adds ENABLE_LTO argument and forwards -DVELOX_ENABLE_LTO=ON to Velox build.
dev/builddeps-veloxbe.sh Wires --enable_lto through to Velox and Gluten CMake configuration.
cpp/velox/operators/serializer/VeloxRowToColumnarConverter.cc Updates HUGEINT byte reversal implementation (uses reverse_copy).
cpp/CMakeLists.txt Adds ENABLE_LTO option and enables IPO/LTO via CheckIPOSupported.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cpp/velox/operators/serializer/VeloxRowToColumnarConverter.cc
Comment thread cpp/CMakeLists.txt Outdated
Comment thread cpp/CMakeLists.txt
Comment thread ep/build-velox/src/build-velox.sh
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 26, 2026 02:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

cpp/velox/operators/serializer/VeloxRowToColumnarConverter.cc:118

  • bytesValue[0] is accessed even when length == 0, which is undefined behavior (and is likely the remaining issue that shows up under LTO/optimizations). The sign-extension logic should only run when at least one byte was read.
      if (length > 0) {
        std::reverse_copy(bytesValue.begin(), bytesValue.begin() + length, bytesValue2);
      }
      if (static_cast<int8_t>(bytesValue[0]) < 0) {
        memset(bytesValue2 + length, 255, 16 - length);
      }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants