Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to make MIGraphX buildable/usable as a fully static build by adjusting how internal libraries are linked and embedded so that required static initialization and generated sources aren’t dropped by the linker.
Changes:
- Introduces an
enable_static_init()CMake helper to force whole-archive linking semantics for static libraries on major platforms. - Switches TF/ONNX protobuf helper libs from
STATICtoOBJECTand adjusts how they’re consumed bymigraphx_tf/migraphx_onnx. - Updates embedding/installation behavior for GPU kernels and enables static-init handling across several component libraries.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
CMakeLists.txt |
Adds global PIC, defines enable_static_init() for static whole-archive semantics. |
src/CMakeLists.txt |
Applies enable_static_init(migraphx) to preserve static init when building migraphx as static. |
src/targets/cpu/CMakeLists.txt |
Applies enable_static_init(migraphx_cpu) for static builds. |
src/targets/gpu/CMakeLists.txt |
Applies enable_static_init(migraphx_gpu) and updates install target list (now includes migraphx_kernels). |
src/onnx/CMakeLists.txt |
Converts onnx-proto to an OBJECT library; adjusts linking and applies static-init handling for migraphx_onnx. |
src/tf/CMakeLists.txt |
Converts tf-proto to an OBJECT library; adjusts linking and applies static-init handling for migraphx_tf. |
cmake/Embed.cmake |
Adds an extra warning-suppression flag and changes embed include dirs to build-interface-only. |
| set(MIGRAPHX_SO_VERSION ${MIGRAPHX_SO_MAJOR_VERSION}.0) | ||
|
|
||
| option( BUILD_SHARED_LIBS "Build as a shared library" ON ) | ||
| option(BUILD_SHARED_LIBS "Build as a shared library" ON ) |
There was a problem hiding this comment.
BUILD_SHARED_LIBS is declared twice in the top-level CMakeLists (once near the top and again here). Having duplicate option(BUILD_SHARED_LIBS ...) definitions can lead to confusing/help-text mismatches and makes it unclear which one is authoritative; consider keeping a single option() definition and removing the duplicate.
| option(BUILD_SHARED_LIBS "Build as a shared library" ON ) |
| endif() | ||
| target_include_directories(${INTERNAL_EMBED_LIB} PRIVATE "${EMBED_DIR}/include") | ||
| target_compile_options(${INTERNAL_EMBED_LIB} PRIVATE -Wno-reserved-identifier -Wno-extern-initializer -Wno-missing-variable-declarations) | ||
| target_compile_options(${INTERNAL_EMBED_LIB} PRIVATE -Wno-reserved-identifier -Wno-extern-initializer -Wno-missing-variable-declarations -Wno-c++11-narrowing) |
There was a problem hiding this comment.
-Wno-c++11-narrowing is added unconditionally. This warning flag is compiler-specific (typically Clang) and can produce “unrecognized option” warnings or failures on other toolchains; consider guarding it with a compiler/flag check (e.g., check_cxx_compiler_flag) or applying it only for Clang.
| target_compile_options(${INTERNAL_EMBED_LIB} PRIVATE -Wno-reserved-identifier -Wno-extern-initializer -Wno-missing-variable-declarations -Wno-c++11-narrowing) | |
| target_compile_options(${INTERNAL_EMBED_LIB} PRIVATE | |
| -Wno-reserved-identifier | |
| -Wno-extern-initializer | |
| -Wno-missing-variable-declarations | |
| $<$<AND:$<COMPILE_LANGUAGE:CXX>,$<CXX_COMPILER_ID:Clang>>:-Wno-c++11-narrowing> | |
| ) |
| rocm_install_targets( | ||
| PRIVATE | ||
| TARGETS migraphx_gpu migraphx_device compile_for_gpu | ||
| TARGETS migraphx_gpu migraphx_device compile_for_gpu migraphx_kernels |
There was a problem hiding this comment.
migraphx_kernels is an embed-generated INTERFACE target that references build-tree generated sources/headers and an internal target (embed_lib_migraphx_kernels). Adding it to rocm_install_targets() is likely to produce an unusable installed target (or export/install errors) unless the generated headers and the internal embed library are also installed/exported. Consider either removing migraphx_kernels from the install set or making it an installable library (and installing its generated headers).
| TARGETS migraphx_gpu migraphx_device compile_for_gpu migraphx_kernels | |
| TARGETS migraphx_gpu migraphx_device compile_for_gpu |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #4817 +/- ##
===========================================
- Coverage 92.49% 92.46% -0.03%
===========================================
Files 583 583
Lines 29562 29564 +2
===========================================
- Hits 27343 27336 -7
- Misses 2219 2228 +9 🚀 New features to boost your workflow:
|
Motivation
Technical Details
Changelog Category
Add a
CHANGELOG.mdentry for any option other thanNot Applicable