Conversation
TheRock#6852 (merged 2026-08-31) removed rocm_smi_lib from ROCm 10.1+. This breaks MORI's GPU topology build (rocm_smi/rocm_smi.h not found). Keep both code paths, version-guarded via ROCM_VERSION from rocm_version.h: - MORI_USE_AMDSMI=1 (ROCm >= 10.1): amd_smi API, socket/processor handles - MORI_USE_AMDSMI=0 (ROCm <= 10.0): rocm_smi API, flat device indices Key API migration: - rsmi_init(0) → amdsmi_init(AMDSMI_INIT_AMD_GPUS) - rsmi_num_monitor_devices → amdsmi_get_socket/processor_handles - rsmi_dev_pci_id_get → amdsmi_get_gpu_device_bdf (BDF struct) - rsmi_is_P2P_accessible → amdsmi_topo_get_p2p_status (4 args) - RSMI_IO_LINK_TYPE → amdsmi_link_type_t - rsmi_status_string → amdsmi_status_code_to_string Uses __has_include for rocm_version.h path portability. Fixes ROCm#638
Formatting only — no logic changes: - check.hpp: align ROCM_SMI_CHECK macro trailing backslashes - gpu.cpp: compress candidates[] array, break long if-statement
QizhouZhang97
left a comment
There was a problem hiding this comment.
Thanks for resolving this issue. Please address the following issue before merging it
| std::vector<amdsmi_socket_handle> sockets(socketCount); | ||
| ROCM_SMI_CHECK(amdsmi_get_socket_handles(&socketCount, sockets.data())); | ||
|
|
||
| uint32_t numGpus = 0; |
There was a problem hiding this comment.
could you check socketCount before you accessing sockets array
|
|
||
| // Bootstrap-only helper: this macro terminates the process and should not be | ||
| // used in per-connection or per-transfer runtime paths. | ||
| #define SYSCALL_RETURN_ZERO(stmt) \ |
| ROCM_SMI_CHECK(amdsmi_get_socket_handles(&socketCount, sockets.data())); | ||
|
|
||
| uint32_t numGpus = 0; | ||
| ROCM_SMI_CHECK(amdsmi_get_processor_handles(sockets[0], &numGpus, nullptr)); |
There was a problem hiding this comment.
the enumeration only walks sockets[0]. In amd_smi a "socket" is one per GPU, not one per host socket. I verified this against the real libamd_smi.so on this 8-GPU box: socket_count=8, each with processor_count=1. So numGpus comes out as 1, seven GPUs disappear, p2ps is empty, and GetGpuByLogicalId(1..7) returns nullptr — which system.cpp:66 dereferences unconditionally. Segfault on every non-zero rank. Fix is to loop over all sockets (or skip sockets entirely and use amdsmi_get_processor_handles_by_type).
… guard 1. gpu.cpp: Loop over ALL sockets to collect processor handles. amd_smi models one socket per GPU (not per host socket), so sockets[0] only sees 1 GPU on an 8-GPU box. Collect from all. 2. gpu.cpp: Guard socketCount == 0 before accessing sockets array. 3. check.hpp: Restore "Bootstrap-only helper" comment on SYSCALL_RETURN_ZERO.
|
@QizhouZhang97 — All three review comments from your Sep 16 review have been addressed in the latest commits:
CI status on latest commit (
The two failures are both on the MI355X_AINIC internode runner reporting "no ROCm-capable device is detected" — a GPU device-passthrough issue on that CI node, not related to this PR's code changes. All builds and all other test configurations passed. Ready for re-review and approval when you get a chance. Thanks! |
TheRock#6852 (merged 2026-08-31) removed rocm_smi_lib from ROCm 10.1+. This breaks MORI's GPU topology build (rocm_smi/rocm_smi.h not found).
Keep both code paths, version-guarded via ROCM_VERSION from rocm_version.h:
Key API migration:
Uses __has_include for rocm_version.h path portability.
Fixes #638
Motivation
MORI build fails on ROCm 10.1+ with
fatal error: 'rocm_smi/rocm_smi.h' file not found.ROCm/TheRock#6852 (merged 2026-08-31) disabled
rocm_smi_libfrom ROCm 10.1+ builds. MORI's GPU topology code (gpu.cpp,gpu.hpp,check.hpp)includes
rocm_smi/rocm_smi.hand uses thersmi_*API, which no longer ships with ROCm 10.1+.Fixes #638
Technical Details
Version-guarded dual code using
ROCM_VERSION_MAJOR/ROCM_VERSION_MINORfromrocm_version.h. Both oldand new code paths live side-by-side:
MORI_USE_AMDSMI=1(ROCm >= 10.1):amdsmi_*API with socket/processor handle modelMORI_USE_AMDSMI=0(ROCm <= 10.0):rsmi_*API with flat device indices (unchanged)Key API migration:
rsmi_init(0)amdsmi_init(AMDSMI_INIT_AMD_GPUS)rsmi_num_monitor_devices(&n)amdsmi_get_socket_handles+amdsmi_get_processor_handlesrsmi_dev_pci_id_get(i, &id)amdsmi_get_gpu_device_bdf(handle, &bdf)rsmi_is_P2P_accessible(i, j, &ok)amdsmi_topo_get_p2p_status(h_i, h_j, &type, &cap)RSMI_IO_LINK_TYPEamdsmi_link_type_trsmi_status_stringamdsmi_status_code_to_stringUses
__has_includeforrocm_version.hpath portability (<rocm-core/rocm_version.h>on ROCm 7.2+,<rocm_version.h>fallback for older).Files changed:
include/mori/application/topology/gpu.hpp— version guard, include switch,amdsmi_link_type_ttypefield
include/mori/application/utils/check.hpp— version guard, dualROCM_SMI_CHECKmacrosrc/application/topology/gpu.cpp— dualLoad()implementations withOpenAmdSmi/OpenRocmSmiTest Plan
MORI_USE_AMDSMI=1pathMORI_USE_AMDSMI=0path (no regression)Test Result
MORI_USE_AMDSMI=1): MORI build +import morismoke test pass — CI run#34608518940
MORI_USE_AMDSMI=0): MORI build passes (downstream torchaudio ABI issue unrelatedto this change)
Submission Checklist