Conversation
| $<TARGET_FILE:migraphx_ref> | ||
| $<TARGET_FILE:migraphx_onnx> | ||
| $<TARGET_FILE:migraphx_tf> | ||
| -Wl,--no-whole-archive |
There was a problem hiding this comment.
Why are we needing this? It should work by just linking migraphx_all_targets migraphx_onnx migraphx_tf
| generate_embed_source(${EMBED_NAME} ${EMBED_DIR} "${PARSE_RELATIVE}" SYMBOLS ${SYMBOLS} FILES ${PARSE_UNPARSED_ARGUMENTS}) | ||
| set(INTERNAL_EMBED_LIB embed_lib_${EMBED_NAME}) | ||
| if(EMBED_USE STREQUAL "LD") | ||
| if(EMBED_USE STREQUAL "LD" OR EMBED_USE STREQUAL "CArrays") |
There was a problem hiding this comment.
CArrays should not create a static library. It should be an object library so it can be linked into the static library.
| if(BUILD_SHARED_LIBS) | ||
| target_link_options(migraphx_gpu PRIVATE "-Wl,--exclude-libs,ALL") | ||
| else() | ||
| target_compile_options(migraphx_gpu PRIVATE -fvisibility=hidden) |
There was a problem hiding this comment.
This doesnt make sense. Why are we doing this>
| MIGRAPHX_ONNX_STATIC_DEFINE | ||
| MIGRAPHX_TF_STATIC_DEFINE | ||
| MIGRAPHX_STATIC_TARGETS | ||
| ) |
There was a problem hiding this comment.
Why are there so many defines? Also the defines should be added using target_compile_definitions.
| elseif(EMBED_USE STREQUAL "LD" OR EMBED_USE STREQUAL "CArrays") | ||
| target_link_libraries(${EMBED_NAME} INTERFACE ${INTERNAL_EMBED_LIB}) | ||
| else() | ||
| target_sources(${EMBED_NAME} INTERFACE $<TARGET_OBJECTS:${INTERNAL_EMBED_LIB}>) |
There was a problem hiding this comment.
Dont remove the object library path.
|
|
||
| if(NOT BUILD_SHARED_LIBS) | ||
| install(TARGETS embed_lib_migraphx_kernels EXPORT migraphx-targets ARCHIVE DESTINATION lib) | ||
| install(TARGETS migraphx_kernels EXPORT migraphx-targets) |
There was a problem hiding this comment.
Embed libraries should not be installed. They should be added to the static library.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #4791 +/- ##
========================================
Coverage 92.49% 92.49%
========================================
Files 583 583
Lines 29561 29562 +1
========================================
+ Hits 27342 27343 +1
Misses 2219 2219
🚀 New features to boost your workflow:
|
| # Static archives only pull in objects that resolve undefined symbols. | ||
| # MIGRAPHX_REGISTER_OP/MIGRAPHX_REGISTER_TARGET use static initializers | ||
| # in translation units with no other referenced symbols, so the linker | ||
| # drops them without --whole-archive. |
There was a problem hiding this comment.
Ideally, the consumers should not do this. Instead this needs to be added to the INTERFACE. Let me test something out to see if it works.
| ) | ||
|
|
||
| if(NOT BUILD_SHARED_LIBS) | ||
| install(TARGETS onnx-proto EXPORT migraphx-targets ARCHIVE DESTINATION lib) |
There was a problem hiding this comment.
We can make this an object library instead of trying to install this extra lib.
| // Load the library without the so_major_version in the name. | ||
| store_target_lib(dynamic_loader(target_name)); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
I dont think this is needed. Claude is just overthinking this.
|
|
||
| if(NOT BUILD_SHARED_LIBS) | ||
| set(_SAVED_EMBED_USE ${EMBED_USE}) | ||
| set(EMBED_USE CArrays) |
There was a problem hiding this comment.
Remove all this. CArrays is already set when BUILD_SHARED_LIBS=Off.
3 participants
Motivation
POC for migraphx driver to compile migraphx statically
Technical Details
to configure, do
cmake -DBUILD_SHARED_LIBS=Off ..Changelog Category
Add a
CHANGELOG.mdentry for any option other thanNot Applicable