rocr: TheRock - Use CMake imported targets for NUMA and hwloc by shwetagkhatri · Pull Request #2598 · ROCm/rocm-systems

shwetagkhatri · 2026-01-13T17:08:28Z

Set BUILD_RPATH and INSTALL_RPATH with $ORIGIN-relative paths to find hwloc/numa libraries at runtime. Conditionally added TheRock rocm_sysdeps directory when detected for bundled dependencies.

Motivation

Technical Details

JIRA ID

Test Plan

Test Result

Submission Checklist

Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

Copilot

Pull request overview

This PR configures CMake RPATH settings to enable runtime library discovery using $ORIGIN-relative paths for the rocrtst test suite. The changes ensure that hwloc and numa libraries can be found at runtime whether bundled or installed system-wide, with additional support for TheRock build environments.

Changes:

Modified CMake RPATH configuration flags to properly separate build and install RPATH behavior
Defined explicit BUILD_RPATH and INSTALL_RPATH with $ORIGIN-relative paths for finding dependencies
Added conditional TheRock-specific RPATH entries for bundled system dependencies (rocm_sysdeps)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-13T17:12:04Z

-## Set RUNPATH to pickup local copy of hwloc
-set_property(TARGET ${ROCRTST} PROPERTY INSTALL_RPATH "$ORIGIN;$ORIGIN/thirdparty/lib;$ORIGIN/../lib/rocrtst/thirdparty/lib" )
+# Base RPATH directories for all builds
+set(_rocrtst_build_rpath "$ORIGIN/../../lib/rocrtst/lib;${CMAKE_CURRENT_SOURCE_DIR}/../../thirdparty/lib")


The BUILD_RPATH mixes a runtime path token ($ORIGIN) with a build-time absolute path (CMAKE_CURRENT_SOURCE_DIR). BUILD_RPATH should use absolute paths for the build tree, not $ORIGIN tokens. Consider using ${PROJECT_BINARY_DIR}/../lib/rocrtst/lib instead of the $ORIGIN-relative path for the first component.

Copilot · 2026-01-13T17:12:04Z

-## Set RUNPATH to pickup local copy of hwloc
-set_property(TARGET ${ROCRTST} PROPERTY INSTALL_RPATH "$ORIGIN;$ORIGIN/thirdparty/lib;$ORIGIN/../lib/rocrtst/thirdparty/lib" )
+# Base RPATH directories for all builds
+set(_rocrtst_build_rpath "$ORIGIN/../../lib/rocrtst/lib;${CMAKE_CURRENT_SOURCE_DIR}/../../thirdparty/lib")


Using CMAKE_CURRENT_SOURCE_DIR in BUILD_RPATH may cause issues if the build directory structure differs from expectations. Consider using CMAKE_CURRENT_BINARY_DIR or PROJECT_BINARY_DIR to reference build-relative paths, or use absolute paths that are valid during the build phase.

Suggested change

set(_rocrtst_build_rpath "$ORIGIN/../../lib/rocrtst/lib;${CMAKE_CURRENT_SOURCE_DIR}/../../thirdparty/lib")

set(_rocrtst_build_rpath "$ORIGIN/../../lib/rocrtst/lib;${CMAKE_CURRENT_BINARY_DIR}/../../thirdparty/lib")

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-16T10:14:02Z

+# Configure RPATH settings before creating executable target
+set(CMAKE_BUILD_WITH_INSTALL_RPATH OFF)
+set(CMAKE_SKIP_BUILD_RPATH FALSE)
+set(CMAKE_BUILD_RPATH_USE_ORIGIN TRUE)


CMAKE_BUILD_RPATH_USE_ORIGIN is available in CMake 3.14+, but this CMakeLists.txt requires only CMake 3.5.0 (line 30). If the target environment uses CMake < 3.14, this setting will be silently ignored, potentially causing runtime linking issues during the build phase. Consider either updating the minimum CMake version requirement or explicitly setting CMAKE_BUILD_RPATH with $ORIGIN paths for compatibility with CMake 3.5.

Suggested change

set(CMAKE_BUILD_RPATH_USE_ORIGIN TRUE)

if (CMAKE_VERSION VERSION_GREATER_EQUAL "3.14")

# Use CMake's built-in support for $ORIGIN in build RPATH on newer versions

set(CMAKE_BUILD_RPATH_USE_ORIGIN TRUE)

else()

# Fallback for CMake < 3.14: explicitly use $ORIGIN-based build RPATH

set(CMAKE_BUILD_RPATH "\$ORIGIN")

endif()

Copilot · 2026-01-16T10:14:02Z

-      message(WARNING "Using system hwloc library: ${HWLOC_LIBRARY}")
-      message(WARNING "  Consider using bundled version to avoid compatibility issues")
-    endif()
+    message(FATAL_ERROR "hwloc library not found. Install libhwloc-dev (Debian/Ubuntu) or hwloc-devel (RHEL/CentOS)")


The hwloc library has been changed from optional ("only needed for certain performance tests") to required with a FATAL_ERROR. This is a breaking change that will prevent builds on systems without hwloc, whereas the previous code allowed fallback behavior. If this is intentional, it should be clearly documented. If hwloc should remain optional for backward compatibility, the FATAL_ERROR should be changed to a WARNING and HAVE_HWLOC should be set to FALSE to allow the build to continue.

Suggested change

message(FATAL_ERROR "hwloc library not found. Install libhwloc-dev (Debian/Ubuntu) or hwloc-devel (RHEL/CentOS)")

message(WARNING "hwloc library not found. hwloc-dependent performance tests will be disabled. Install libhwloc-dev (Debian/Ubuntu) or hwloc-devel (RHEL/CentOS) to enable them.")

set(HAVE_HWLOC FALSE)

Copilot · 2026-01-16T10:14:03Z

+  find_library(HWLOC_LIBRARY 
+    NAMES hwloc
+    HINTS
+      ${CMAKE_PREFIX_PATH}/lib
+      ${CMAKE_INSTALL_PREFIX}/lib
+      ${PROJECT_BINARY_DIR}/../thirdparty/lib
+      ${PROJECT_SOURCE_DIR}/thirdparty/lib
+    PATH_SUFFIXES lib lib64
+  )


The search for hwloc library includes paths like PROJECT_BINARY_DIR/../thirdparty/lib and PROJECT_SOURCE_DIR/thirdparty/lib, but these assume a specific directory structure. When combined with CMAKE_INSTALL_RPATH_USE_LINK_PATH set to TRUE without explicit INSTALL_RPATH configuration, this may not correctly propagate the RPATH for bundled libraries. The old code explicitly handled this with "$ORIGIN/thirdparty/lib" paths. Consider adding explicit CMAKE_INSTALL_RPATH settings or verifying that CMAKE_INSTALL_RPATH_USE_LINK_PATH will correctly detect and use these library paths.

Copilot · 2026-01-16T10:14:03Z

+# CMake will automatically set RPATH based on linked library locations
+# when CMAKE_INSTALL_RPATH_USE_LINK_PATH is TRUE
 set_property(TARGET ${ROCRTST} PROPERTY LINK_FLAGS "-Wl,--enable-new-dtags")


The comment claims that CMake will automatically set RPATH based on linked library locations when CMAKE_INSTALL_RPATH_USE_LINK_PATH is TRUE, but no explicit CMAKE_INSTALL_RPATH is set. This may cause runtime failures for the installed binary when it cannot find hwloc or numa libraries that were found in bundled thirdparty/lib directories during build. The old code explicitly set INSTALL_RPATH to include "$ORIGIN/thirdparty/lib" and "$ORIGIN/../lib/rocrtst/thirdparty/lib" which matches the install location at line 625. Consider explicitly setting CMAKE_INSTALL_RPATH to include the thirdparty library paths relative to the binary installation location.

Suggested change

# CMake will automatically set RPATH based on linked library locations

# when CMAKE_INSTALL_RPATH_USE_LINK_PATH is TRUE

set_property(TARGET ${ROCRTST} PROPERTY LINK_FLAGS "-Wl,--enable-new-dtags")

# Ensure the installed binary can locate bundled third-party libraries via RPATH.

# Use CMAKE_INSTALL_RPATH_USE_LINK_PATH for standard libraries and add an explicit

# INSTALL_RPATH entry for the third-party libs installed under lib/rocrtst/lib.

set(CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE)

set_target_properties(${ROCRTST} PROPERTIES

LINK_FLAGS "-Wl,--enable-new-dtags"

INSTALL_RPATH "$ORIGIN/../lib/rocrtst/lib"

)

Copilot · 2026-01-16T10:14:03Z

+    add_library(numa_imported INTERFACE IMPORTED)
+    set_target_properties(numa_imported PROPERTIES
+      INTERFACE_INCLUDE_DIRECTORIES "${NUMA_PC_INCLUDE_DIRS}"
+      INTERFACE_LINK_LIBRARIES "${NUMA_PC_LIBRARIES}"
+      INTERFACE_LINK_DIRECTORIES "${NUMA_PC_LIBRARY_DIRS}"
+    )
+    set(NUMA_TARGET numa_imported)
+    set(HAVE_NUMA TRUE)
+    message(STATUS "Found NUMA via pkg-config: ${NUMA_PC_LIBRARIES}")
+    message(STATUS "  Include dirs: ${NUMA_PC_INCLUDE_DIRS}")
+    message(STATUS "  Library dirs: ${NUMA_PC_LIBRARY_DIRS}")
  endif()
-  set(HAVE_NUMA TRUE)
-else()
-  # Fallback to manual detection
-  find_library(NUMA_LIBRARY NAMES numa)
-  if(NUMA_LIBRARY)
-    set(NUMA_LIBS ${NUMA_LIBRARY})
-    message(STATUS "Found numa library (fallback): ${NUMA_LIBRARY}")
+endif()
+
+if(NOT HAVE_NUMA)
+  find_package(NUMA)
+  if(NUMA_FOUND)
+    add_library(numa_imported INTERFACE IMPORTED)
+    set_target_properties(numa_imported PROPERTIES
+      INTERFACE_INCLUDE_DIRECTORIES "${NUMA_INCLUDE_DIRS}"
+      INTERFACE_LINK_LIBRARIES "${NUMA_LIBRARIES}"
+    )
+    set(NUMA_TARGET numa_imported)
    set(HAVE_NUMA TRUE)
+    message(STATUS "Found NUMA package: ${NUMA_LIBRARIES}")
  else()
-    message(WARNING "NUMA library not found. Building without numa support.")
-    message(WARNING "  Install libnuma-dev (Debian/Ubuntu) or numactl-devel (RHEL/CentOS)")
-    set(NUMA_LIBS "")
-    set(HAVE_NUMA FALSE)
-    # Exclude numa-dependent files from build
-    list(FILTER performanceSources EXCLUDE REGEX ".*memory_async_copy.*\\.cc$")
+    # Last resort: manual detection
+    find_library(NUMA_LIBRARY NAMES numa)
+    if(NUMA_LIBRARY)
+      add_library(numa_imported INTERFACE IMPORTED)
+      set_target_properties(numa_imported PROPERTIES
+        INTERFACE_LINK_LIBRARIES "${NUMA_LIBRARY}"
+      )
+      set(NUMA_TARGET numa_imported)


Creating multiple imported targets with the same name "numa_imported" in different code paths will cause CMake errors if more than one path is executed. Although the code has conditional guards (if/else), it's possible for this to cause issues during reconfiguration or if the logic changes. Consider using unique names for each fallback path (e.g., numa_imported_pc, numa_imported_find_package, numa_imported_manual) or ensure that target existence is checked before creating.

Copilot · 2026-01-16T10:14:03Z

+    add_library(hwloc_imported INTERFACE IMPORTED)
+    set_target_properties(hwloc_imported PROPERTIES
+      INTERFACE_INCLUDE_DIRECTORIES "${HWLOC_PC_INCLUDE_DIRS}"
+      INTERFACE_LINK_LIBRARIES "${HWLOC_PC_LIBRARIES}"
+      INTERFACE_LINK_DIRECTORIES "${HWLOC_PC_LIBRARY_DIRS}"
+    )
+    set(HWLOC_TARGET hwloc_imported)
+    set(HAVE_HWLOC TRUE)
+    message(STATUS "Found hwloc via pkg-config: ${HWLOC_PC_LIBRARIES}")
+    message(STATUS "  Include dirs: ${HWLOC_PC_INCLUDE_DIRS}")
+    message(STATUS "  Library dirs: ${HWLOC_PC_LIBRARY_DIRS}")
  endif()
 endif()

-# If not found via pkg-config, check for bundled version (any version)
-if(NOT HWLOC_FOUND)
-  # Use file(GLOB) to find any libhwloc.so* file regardless of version
-  file(GLOB HWLOC_LIBRARY "${CMAKE_CURRENT_SOURCE_DIR}/../../thirdparty/lib/libhwloc.so*")
+if(NOT HAVE_HWLOC)
+  # Search for hwloc library using standard search paths
+  find_library(HWLOC_LIBRARY 
+    NAMES hwloc
+    HINTS
+      ${CMAKE_PREFIX_PATH}/lib
+      ${CMAKE_INSTALL_PREFIX}/lib
+      ${PROJECT_BINARY_DIR}/../thirdparty/lib
+      ${PROJECT_SOURCE_DIR}/thirdparty/lib
+    PATH_SUFFIXES lib lib64
+  )
+
  if(HWLOC_LIBRARY)
-    list(GET HWLOC_LIBRARY 0 HWLOC_LIBRARY)  # Take first match if multiple versions exist
-    set(HWLOC_FOUND TRUE)
-    message(STATUS "Found bundled hwloc library: ${HWLOC_LIBRARY}")
+    # Create a proper IMPORTED SHARED library so CMake auto-handles RPATH
+    add_library(hwloc_imported SHARED IMPORTED)
+    set_target_properties(hwloc_imported PROPERTIES
+      IMPORTED_LOCATION "${HWLOC_LIBRARY}"
+    )
+    set(HWLOC_TARGET hwloc_imported)


Creating multiple imported targets with the same name "hwloc_imported" in different code paths will cause CMake errors if both paths execute. Although the code has conditional guards (if/else), consider using unique names for each fallback path or ensuring that target existence is checked before creating. Note that line 558 creates an INTERFACE IMPORTED target while line 586 creates a SHARED IMPORTED target, which have different semantics.

Copilot · 2026-01-16T10:14:04Z

@@ -577,4 +624,4 @@ install(TARGETS ${ROCRTST}

 install ( DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/../../thirdparty/lib DESTINATION lib/rocrtst )



The PR description mentions "conditionally added TheRock rocm_sysdeps directory when detected for bundled dependencies", but there is no code in this diff that searches for or adds a rocm_sysdeps directory. This suggests either incomplete implementation or the description is referring to changes in other files not included in this PR.

Suggested change

# Conditionally install TheRock rocm_sysdeps directory when present for bundled dependencies

set(ROCM_SYSDEPS_SRC_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../../thirdparty/lib/rocm_sysdeps")

if (EXISTS "${ROCM_SYSDEPS_SRC_DIR}")

install ( DIRECTORY "${ROCM_SYSDEPS_SRC_DIR}" DESTINATION lib/rocrtst )

endif()

jharryma · 2026-01-22T00:14:54Z

Rocrtst test are running on theRock: https://github.com/ROCm/TheRock/actions/runs/21210936024/job/61035359766?pr=2873#step:10:477

They are only timing out due to the low time limit set on theRock test configs. I will increase them and rerun to validate the result of all the tests.

This PR should be ready to review as is though!

jharryma · 2026-01-22T23:07:47Z

All tests passed on theRock: https://github.com/ROCm/TheRock/actions/runs/21232779979/job/61108683343?pr=2873#step:10:2014

jharryma

rocrtst tests are passing on TheRock PR (ROCm/TheRock#2873) with this rocm-systems branch pulled in:
https://github.com/ROCm/TheRock/actions/runs/21232779979/job/61108683343?pr=2873#step:10:2014

Use SHARED IMPORTED targets for proper RPATH handling, prioritize pkg-config over bundled binaries, removed manual symlink workarounds

…g fallback Co-authored-by: Jessey Harrymanoharan <Jessey.Harrymanoharan@amd.com>

github-actions · 2026-05-14T18:13:06Z

This pull request has been inactive for 25 days and will be marked as stale.

If you would like to keep this PR open, please:

Add new commits
Add a comment explaining why it should remain open

This PR will be automatically closed in 5 days if no further activity occurs.

github-actions · 2026-05-19T22:08:59Z

This pull request has been automatically closed due to inactivity (30 days with no updates).

If you'd like to continue working on this, feel free to reopen the PR or create a new one.

shwetagkhatri requested a review from cfreeamd as a code owner January 13, 2026 17:08

shwetagkhatri requested review from Copilot and jharryma January 13, 2026 17:08

github-actions Bot added the project: rocr-runtime label Jan 13, 2026

Copilot started reviewing on behalf of shwetagkhatri January 13, 2026 17:09 View session

Copilot AI reviewed Jan 13, 2026

View reviewed changes

systems-assistant Bot added the organization: ROCm label Jan 13, 2026

shwetagkhatri force-pushed the users/shwetak/TheRockSupport branch 5 times, most recently from 0072eac to 2bf20da Compare January 16, 2026 10:02

shwetagkhatri changed the title ~~Add RPATH for runtime library discovery~~ rocr: TheRock - Use CMake imported targets for NUMA and hwloc Jan 16, 2026

shwetagkhatri requested a review from Copilot January 16, 2026 10:09

Copilot started reviewing on behalf of shwetagkhatri January 16, 2026 10:10 View session

Copilot AI reviewed Jan 16, 2026

View reviewed changes

jharryma force-pushed the users/shwetak/TheRockSupport branch from 2bf20da to 20084de Compare January 16, 2026 16:06

shwetagkhatri force-pushed the users/shwetak/TheRockSupport branch 7 times, most recently from 6f658cb to faed7f6 Compare January 17, 2026 11:33

jharryma mentioned this pull request Jan 21, 2026

[CI] Build and Enable rocrtst for ROCR-Runtime on theRock ROCm/TheRock#2498

Open

jharryma approved these changes Jan 23, 2026

View reviewed changes

jharryma force-pushed the users/shwetak/TheRockSupport branch from 98eeb10 to 2124909 Compare January 28, 2026 06:26

rocrtst: TheRock - Refactor NUMA/hwloc dependency discovery

7f9b333

Use SHARED IMPORTED targets for proper RPATH handling, prioritize pkg-config over bundled binaries, removed manual symlink workarounds

rocrtst: TheRock support: Prefer hwloc CMake package before pkg-confi…

0b4a1cd

…g fallback Co-authored-by: Jessey Harrymanoharan <Jessey.Harrymanoharan@amd.com>

jharryma force-pushed the users/shwetak/TheRockSupport branch from 2124909 to 0b4a1cd Compare January 29, 2026 08:18

shwetagkhatri requested a review from dayatsin-amd January 29, 2026 17:23

dayatsin-amd approved these changes Feb 4, 2026

View reviewed changes

cfreeamd approved these changes Feb 4, 2026

View reviewed changes

Merge branch 'develop' into users/shwetak/TheRockSupport

396ba7f

shwetagkhatri mentioned this pull request Feb 5, 2026

rocrtst: Disabled NUMA async copy build #3044

Merged

1 task

github-actions Bot added the Stale PR has no activity for 25+ days label May 14, 2026

github-actions Bot closed this May 19, 2026

	set(_rocrtst_build_rpath "$ORIGIN/../../lib/rocrtst/lib;${CMAKE_CURRENT_SOURCE_DIR}/../../thirdparty/lib")
	set(_rocrtst_build_rpath "$ORIGIN/../../lib/rocrtst/lib;${CMAKE_CURRENT_BINARY_DIR}/../../thirdparty/lib")

-set(CMAKE_BUILD_RPATH_USE_ORIGIN TRUE)
+if (CMAKE_VERSION VERSION_GREATER_EQUAL "3.14")
+  # Use CMake's built-in support for $ORIGIN in build RPATH on newer versions
+  set(CMAKE_BUILD_RPATH_USE_ORIGIN TRUE)
+else()
+  # Fallback for CMake < 3.14: explicitly use $ORIGIN-based build RPATH
+  set(CMAKE_BUILD_RPATH "\$ORIGIN")
+endif()

	message(FATAL_ERROR "hwloc library not found. Install libhwloc-dev (Debian/Ubuntu) or hwloc-devel (RHEL/CentOS)")
	message(WARNING "hwloc library not found. hwloc-dependent performance tests will be disabled. Install libhwloc-dev (Debian/Ubuntu) or hwloc-devel (RHEL/CentOS) to enable them.")
	set(HAVE_HWLOC FALSE)

-# CMake will automatically set RPATH based on linked library locations
-# when CMAKE_INSTALL_RPATH_USE_LINK_PATH is TRUE
-set_property(TARGET ${ROCRTST} PROPERTY LINK_FLAGS "-Wl,--enable-new-dtags")
+# Ensure the installed binary can locate bundled third-party libraries via RPATH.
+# Use CMAKE_INSTALL_RPATH_USE_LINK_PATH for standard libraries and add an explicit
+# INSTALL_RPATH entry for the third-party libs installed under lib/rocrtst/lib.
+set(CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE)
+set_target_properties(${ROCRTST} PROPERTIES
+  LINK_FLAGS "-Wl,--enable-new-dtags"
+  INSTALL_RPATH "$ORIGIN/../lib/rocrtst/lib"
+)

		@@ -577,4 +624,4 @@ install(TARGETS ${ROCRTST}

		install ( DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/../../thirdparty/lib DESTINATION lib/rocrtst )

+# Conditionally install TheRock rocm_sysdeps directory when present for bundled dependencies
+set(ROCM_SYSDEPS_SRC_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../../thirdparty/lib/rocm_sysdeps")
+if (EXISTS "${ROCM_SYSDEPS_SRC_DIR}")
+  install ( DIRECTORY "${ROCM_SYSDEPS_SRC_DIR}" DESTINATION lib/rocrtst )
+endif()

Conversation

shwetagkhatri commented Jan 13, 2026

Motivation

Technical Details

JIRA ID

Test Plan

Test Result

Submission Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

jharryma commented Jan 22, 2026

Uh oh!

jharryma commented Jan 22, 2026

Uh oh!

jharryma left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 14, 2026

Uh oh!

github-actions Bot commented May 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants