Skip to content

fix[build]: default build from source with -march=native#184

Merged
feihongxu0824 merged 2 commits intomainfrom
fix/build_with_native
Feb 28, 2026
Merged

fix[build]: default build from source with -march=native#184
feihongxu0824 merged 2 commits intomainfrom
fix/build_with_native

Conversation

@feihongxu0824
Copy link
Collaborator

No description provided.

@greptile-apps
Copy link

greptile-apps bot commented Feb 28, 2026

Greptile Summary

This PR adds a new ENABLE_NATIVE CMake option (defaulting to ON) that builds with -march=native for optimal performance on the build machine. While this improves performance for local builds from source, it introduces a critical portability issue for PyPI wheel distribution:

  • PyPI wheels are built on self-hosted runners with specific CPUs (linux_x64, linux_arm64)
  • With -march=native, wheels become optimized for those specific build machines
  • Users with different/older CPUs will encounter illegal instruction crashes when using these wheels
  • The wheel build workflow (.github/workflows/build_wheel.yml) doesn't override this setting

Required action: Add ENABLE_NATIVE = "OFF" to pyproject.toml under [tool.scikit-build.cmake.define] or modify wheel build workflows to explicitly disable it. The main CI workflow already handles architecture flags correctly (see ENABLE_ZEN3 usage in main.yml).

Confidence Score: 1/5

  • This PR introduces a critical issue that will break PyPI wheel distribution for users with different CPUs than the build machines
  • The default ENABLE_NATIVE=ON will cause wheels to be built with -march=native on self-hosted runners, making them incompatible with different CPU architectures. This is a breaking change that affects all users installing from PyPI.
  • cmake/option.cmake requires immediate attention - the default ENABLE_NATIVE=ON breaks wheel portability

Important Files Changed

Filename Overview
cmake/option.cmake Added ENABLE_NATIVE option (default ON) to build with -march=native, which breaks wheel distribution portability

Last reviewed commit: e298842

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

option(ENABLE_EMERALDRAPIDS "Enable Intel Emerald Rapids Server CPU microarchitecture" OFF)
option(ENABLE_GRANITERAPIDS "Enable Intel Granite Rapids Server CPU microarchitecture" OFF)

option(ENABLE_NATIVE "Enable native CPU microarchitecture" ON)
Copy link

Choose a reason for hiding this comment

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

Defaulting ENABLE_NATIVE to ON breaks PyPI wheel distribution. Wheels built on self-hosted runners (.github/workflows/build_wheel.yml) will use -march=native, making them CPU-specific to the build machine. Users with different CPUs will get illegal instruction errors.

Need to either:

  1. Set ENABLE_NATIVE = "OFF" in pyproject.toml under [tool.scikit-build.cmake.define]
  2. Or add --config-settings='cmake.define.ENABLE_NATIVE="OFF"' to wheel build workflows

Comment on lines 44 to 51
if(NOT ENABLE_NATIVE)
foreach(opt IN LISTS ARCH_OPTIONS)
if(${opt})
set(AUTO_DETECT_ARCH OFF)
break()
endif()
endforeach()
endif()
Copy link

Choose a reason for hiding this comment

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

When ENABLE_NATIVE=ON (default), this loop is skipped, meaning explicit architecture options like ENABLE_SKYLAKE=ON are ignored. Users must explicitly set ENABLE_NATIVE=OFF to use specific architectures. Document this priority in the build guide.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Collaborator

@egolearner egolearner left a comment

Choose a reason for hiding this comment

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

LGTM

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants