Skip to content

Comments

Handle ROCTX using find_path and find_library instead of Findroctx.cmake module#513

Merged
jsandham merged 2 commits intoROCm:developfrom
jsandham:roctx-fix2
Jun 20, 2025
Merged

Handle ROCTX using find_path and find_library instead of Findroctx.cmake module#513
jsandham merged 2 commits intoROCm:developfrom
jsandham:roctx-fix2

Conversation

@jsandham
Copy link
Contributor

@jsandham jsandham commented Jun 18, 2025

Summary of proposed changes:

  • This better handles supporting rocTX in rocSPARSE given rocTRACER does not ship a roctxConfig.cmake file and so we cannot use find_package(roctracer) from rocSPARSE.
  • Previously we tried using our own custom Findroctx.cmake module file included as part of rocSPARSE, but this was deemed not appropriate going forward. See Better handle building rocSPARSE with ROC-TX #484
  • Instead I have gone back to doing something similar to how MIopen and rocBLAS handle incorporating rocTX by using find_path and find_library to search for the roctracer header files and roctx shared library.

@jsandham jsandham requested review from TorreZuk and cgmb June 18, 2025 19:05
@TorreZuk TorreZuk requested a review from Copilot June 18, 2025 21:17
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR replaces the custom Findroctx.cmake module with direct find_path/find_library calls to locate rocTX (roctracer) headers and library, aligning rocSPARSE with how other ROCm components integrate rocTX.

  • Introduce ROCTX_PATH cache variable and use find_path/find_library in library/CMakeLists.txt
  • Replace hardcoded -lroctx64 linking in tests and benchmarks with conditional linking to discovered library
  • Emit clear status and warning messages when rocTX components are found or missing

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
library/CMakeLists.txt Added ROCTX_PATH, used find_path/find_library, conditional include/link logic and messages.
clients/tests/CMakeLists.txt Updated rocsparse-test to include/link based on found rocTX, wrapped in additional guards.
clients/benchmarks/CMakeLists.txt Updated rocsparse-bench similarly to use discovered rocTX include and library paths.
Comments suppressed due to low confidence (4)

library/CMakeLists.txt:128

  • [nitpick] Inconsistent casing in comment: change 'rocTx' to 'roctx' to match the actual library name.
      # Link the rocTx lib

library/CMakeLists.txt:100

  • ROCTX_PATH is introduced here but not mentioned in the project README or CMake usage docs; consider documenting its purpose and valid values.
    set(ROCTX_PATH "" CACHE STRING "Path to the roctx library (directory containing libroctx64.so)")

clients/tests/CMakeLists.txt:428

  • [nitpick] Indent this endif() to align with the opening if(NOT WIN32) for clearer structure and readability.
endif()

clients/benchmarks/CMakeLists.txt:245

  • [nitpick] Indent this endif() to match its corresponding if(NOT WIN32) block for consistent formatting.
endif()

target_link_libraries(rocsparse PRIVATE -lroctx64)
target_compile_definitions( rocsparse PRIVATE ROCSPARSE_BUILT_WITH_ROCTX )

set(ROCTX_PATH "" CACHE STRING "Path to the roctx library (directory containing libroctx64.so)")
Copy link

Copilot AI Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use CACHE TYPE PATH instead of STRING for ROCTX_PATH so CMake GUI tools recognize it as a filesystem path.

Suggested change
set(ROCTX_PATH "" CACHE STRING "Path to the roctx library (directory containing libroctx64.so)")
set(ROCTX_PATH "" CACHE PATH "Path to the roctx library (directory containing libroctx64.so)")

Copilot uses AI. Check for mistakes.
@@ -416,8 +416,14 @@ endif()

if (NOT WIN32)
if (BUILD_WITH_ROCTX)
Copy link

Copilot AI Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The rocTX integration logic is duplicated between the tests and benchmarks; consider extracting it into a reusable CMake function or macro to reduce duplication.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@TorreZuk TorreZuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note I thought what you had originally had was okay, but this mostly matches rocBLAS. Ruban can look but that will be next week, you probably don't want to wait. I guess if the ROCTX_PATH is set as documented then a lot of the hints are off for relative directories but that is fine if you cover most installs or fail gracefully.

rocm-cmake could have been good location for temporary common workaround too? @cgmb ? Expect too late for that.

Ignore any of my AI pair programmer review comments that don't make sense.

@jsandham jsandham merged commit 502a46e into ROCm:develop Jun 20, 2025
20 checks passed
jsandham added a commit to jsandham/rocSPARSE that referenced this pull request Jun 20, 2025
…ake module (ROCm#513)

* Alternate way to handle building rocsparse with roctx since we cannot use Findroctx.cmake modules
ammallya pushed a commit that referenced this pull request Oct 28, 2025
…ake module (#513)

* Alternate way to handle building rocsparse with roctx since we cannot use Findroctx.cmake modules

[ROCm/rocSPARSEcommit: 502a46e]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants