Repository navigation
core: gate amdsmi WSL backend on THEROCK_ENABLE_WSL_ROCDXG - #7635
Merged
Merged
Conversation
The amdsmi WSL backend was enabled unconditionally, so default builds shipped a backend that dlopens librocdxg.so while the wsl-rocdxg subproject that produces that library stayed off. TheRock already has a switch for this component; follow it instead of forcing the backend on. Enabling the backend also makes amdsmi require rocr-runtime/libhsakmt's rocdxg_smi.h at configure time, which is a build-time dependency default builds should not take on. Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
Contributor
Author
|
This PR only needs to be merged if this PR doesn't make it in: ROCm/rocm-systems#10436 |
Contributor
Author
|
Doesn't look like my build time dependency removal changes will make it in 10.1, would be wise to merge this |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #7102, which added
-DENABLE_WSL_BACKEND=ONto theamdsmisubproject unconditionally.TheRock already has a switch for this component,
THEROCK_ENABLE_WSL_ROCDXG(feature groupWSL, off in default/CI builds). With the backend forced on, the two disagree:dlopenslibrocdxg.so, while thewsl-rocdxgsubproject that produces that library is not built. Thedlopenjust fails at runtime, so nothing breaks, but the code is dead weight in every default build.ENABLE_WSL_BACKEND=ONis not dependency-free on the amdsmi side. It has aFATAL_ERRORgate requiringrocr-runtime/libhsakmt/include/hsakmt/rocdxg_smi.h, resolved through a hardcoded sibling path. That works today only becauserocm-systemsis checked out whole, and it is a build-time dependency default builds should not take on.This makes the amdsmi flag follow TheRock's existing switch, matching the
-DBUILD_TESTS=${THEROCK_BUILD_TESTING}pattern on the adjacent line. Turning onTHEROCK_ENABLE_WSL_ROCDXGnow builds both the bridge library and the amdsmi backend that consumes it.JIRA ID
ROCM-28065
Test plan
Verified
THEROCK_ENABLE_WSL_ROCDXGis aCACHE BOOLcreated bytherock_add_feature, and thattherock_finalize_features()(top-levelCMakeLists.txt:277) runs well beforeadd_subdirectory(core)(CMakeLists.txt:503), so the variable is in scope at the point of use.Substitution checked for every state:
THEROCK_ENABLE_WSL_ROCDXGOFF-DENABLE_WSL_BACKEND=OFFON-DENABLE_WSL_BACKEND=ON-DENABLE_WSL_BACKEND=(falsey; amdsmi'soption(... OFF)default preserved, no error)THEROCK_ENABLE_WSL_ROCDXG=ONpropagatesENABLE_WSL_BACKEND=ONto amdsmiif(NOT WIN32)guard)pre-commit run --files core/CMakeLists.txt docs/development/wsl_rocdxg.mdpassesFor reference, in run 32498518713 the
compiler-runtimestage built amdsmi with the backend on andcore-amdsmi_lib_genericgrew from 29,896,091 to 30,051,269 bytes (+151 KB) versusmain. That delta is what default builds stop carrying with this change.