[hipsolver] Refactor build system by ibrahimw1 · Pull Request #2 · ibrahimw1/rocm-libraries

ibrahimw1 · 2026-03-10T21:30:32Z

Motivation

hipSOLVER's CMake build system used legacy directory-scoped patterns (global include_directories(),
link_directories(), add_definitions()) that cause scope pollution, make dependencies implicit, and complicate
maintenance. This refactoring modernizes the build system to use target-centric design with explicit dependency
propagation.

Goals:

Eliminate global directory-scoped commands
Make all dependencies explicit via target properties
Enable proper transitive dependency propagation
Maintain byte-for-byte identical build artifacts
Improve build system maintainability and clarity

Technical Details

Root CMakeLists.txt

Converted to thin orchestration layer (32 lines)
Delegates to library/ and clients/ subdirectories
Removed legacy global commands
Added modern CMake project structure

library/CMakeLists.txt

Converted hipsolver target to modern target-centric design
Replaced include_directories() with target_include_directories() (PUBLIC/PRIVATE scoping)
Replaced link_libraries() with target_link_libraries() with explicit visibility
Added proper INTERFACE_INCLUDE_DIRECTORIES for downstream consumers
Configured install rules with namespace (roc::hipsolver)
Generated modern CMake package files for find_package(hipsolver) support

clients/CMakeLists.txt

Modernized client executables (hipsolver-test, hipsolver-bench, etc.)
Replaced global commands with target-specific properties
Added explicit dependency chains
Fixed include path propagation

CMakePresets.json

Added workflow presets for common build configurations
Configured CMAKE_CXX_COMPILER to match ROCm toolchain
Fixed compiler mismatch issues in Docker builds

Key Improvements

Explicit dependencies: All include paths, link libraries, and compile definitions are now explicit on
targets
Scope safety: No global state pollution - each target declares exactly what it needs
Transitive propagation: PUBLIC/INTERFACE/PRIVATE visibility enables correct downstream consumption
Namespace: Install exports use roc::hipsolver namespace for clarity
Package config: Modern CMake package files support find_package(hipsolver) in downstream projects

Validation Results

Configuration equivalence: ✅ PASS

Compile commands: identical
Link commands: identical
Cache variables: equivalent (expected modernization differences only)

Build artifact equivalence: ✅ PASS

Binary sizes: byte-identical
Symbol tables: identical
Dynamic linking: identical
ABI compatibility: verified

Install tree equivalence: ✅ PASS

Installed files: identical
CMake package files: valid and functional
Package contents: equivalent

Test Plan

Test Result

Submission Checklist

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

ibrahimw1 · 2026-03-16T17:18:21Z

 target_link_libraries(hipsolver-bench PRIVATE
-  ${LAPACK_LIBRARIES}
-  hipsolver-common
-  clients-common
-  roc::hipsolver
+    ${LAPACK_LIBRARIES}
+    hipsolver-clients-common
+    roc::hipsolver
 )


Remove redundant direct link to roc::hipsolver

The roc::hipsolver target is already linked as a PUBLIC dependency of hipsolver-clients-common (defined at clients/CMakeLists.txt line 61). Listing it again here creates redundancy in the dependency graph. The transitive link from hipsolver-clients-common already propagates roc::hipsolver to hipsolver-bench, making the explicit link unnecessary.

Recommended action: Remove roc::hipsolver from this target_link_libraries() call to simplify the dependency graph and avoid duplication.

Suggested change

target_link_libraries(hipsolver-bench PRIVATE

${LAPACK_LIBRARIES}

hipsolver-common

clients-common

roc::hipsolver

${LAPACK_LIBRARIES}

hipsolver-clients-common

roc::hipsolver

)

target_link_libraries(hipsolver-bench PRIVATE

${LAPACK_LIBRARIES}

hipsolver-clients-common

)

Generated by Claude Code

ibrahimw1 · 2026-03-16T17:18:23Z

 target_link_libraries(hipsolver-test PRIVATE
-  ${LAPACK_LIBRARIES}
-  hipsolver-common
-  clients-common
-  $<IF:$<TARGET_EXISTS:GTest::gtest>,GTest::gtest,GTest::GTest>
-  Threads::Threads
-  roc::hipsolver
+    ${LAPACK_LIBRARIES}
+    hipsolver-clients-common
+    $<IF:$<TARGET_EXISTS:GTest::gtest>,GTest::gtest,GTest::GTest>
+    Threads::Threads
+    roc::hipsolver
 )


Remove redundant direct link to roc::hipsolver

Same redundancy as in the benchmarks: roc::hipsolver is already linked transitively through hipsolver-clients-common (which declares it PUBLIC at clients/CMakeLists.txt:61). The explicit link here is unnecessary.

Recommended action: Remove roc::hipsolver from this target_link_libraries() call to eliminate the redundant dependency declaration.

Suggested change

target_link_libraries(hipsolver-test PRIVATE

${LAPACK_LIBRARIES}

hipsolver-common

clients-common

$<IF:$<TARGET_EXISTS:GTest::gtest>,GTest::gtest,GTest::GTest>

Threads::Threads

roc::hipsolver

${LAPACK_LIBRARIES}

hipsolver-clients-common

$<IF:$<TARGET_EXISTS:GTest::gtest>,GTest::gtest,GTest::GTest>

Threads::Threads

roc::hipsolver

)

target_link_libraries(hipsolver-test PRIVATE

${LAPACK_LIBRARIES}

hipsolver-clients-common

$<IF:$<TARGET_EXISTS:GTest::gtest>,GTest::gtest,GTest::GTest>

Threads::Threads

)

Generated by Claude Code

ibrahimw1 · 2026-03-16T17:18:24Z

-      $<BUILD_INTERFACE:${CUDA_INCLUDE_DIRS}>
-  )
+if(NOT HIPSOLVER_ENABLE_CUDA)
+    target_link_libraries(hipsolver-bench PRIVATE hip::host)


Remove redundant link to hip::host (already transitively available)

The hip::host target is already linked as a PUBLIC dependency of roc::hipsolver (see library/src/CMakeLists.txt line 109). Since hipsolver-bench links hipsolver-clients-common, which in turn links roc::hipsolver PUBLIC, the hip::host target is already transitively available. This direct link is redundant.

Recommended action: Remove this hip::host link to keep the dependency graph clean. Note that the same redundancy exists in the gtest CMakeLists.txt at line 119 and should be addressed there as well.

Generated by Claude Code

@ibrahimw1 hip::host and hip::device should never be public deps. I thought we had rule for this.

ibrahimw1 · 2026-03-16T17:18:24Z

-else()
-  target_compile_definitions(hipsolver-test PRIVATE __HIP_PLATFORM_NVIDIA__)
+if(NOT HIPSOLVER_ENABLE_CUDA)
+    target_link_libraries(hipsolver-test PRIVATE hip::host)


Remove redundant link to hip::host (already transitively available)

The hip::host target is already a PUBLIC transitive dependency of roc::hipsolver, which is linked through hipsolver-clients-common. This direct link is redundant.

Recommended action: Remove this hip::host link to simplify the dependency graph. Also review the CUSTOM_TARGET link at line 122 for the same potential redundancy.

Generated by Claude Code

ibrahimw1 · 2026-03-16T17:18:25Z


 if(NOT DEFINED CMAKE_Fortran_COMPILER AND NOT DEFINED ENV{FC})
-  set(CMAKE_Fortran_COMPILER  "gfortran")
+    set(CMAKE_Fortran_COMPILER "gfortran")


Hard-coding CMAKE_Fortran_COMPILER violates CMake conventions

Setting CMAKE_Fortran_COMPILER to a hard-coded value ("gfortran") in CMakeLists.txt overrides CMake's standard compiler detection mechanism. CMake automatically searches for a Fortran compiler when enable_language(Fortran) is called. The standard approach for controlling compiler selection is to use the FC environment variable or pass -DCMAKE_Fortran_COMPILER=... on the command line.

This set() call prevents users from selecting their preferred Fortran compiler and makes the build less flexible.

Recommended action: Remove this block entirely and let CMake handle compiler detection through enable_language(Fortran). If a default is needed for your development environment, document it in CMakePresets.json or a toolchain file rather than hard-coding it in the main CMakeLists.txt.

Generated by Claude Code

Or use a toolchain file

ibrahimw1 · 2026-03-16T17:18:26Z

+option(HIPSOLVER_ENABLE_BENCHMARKS "Build hipSOLVER benchmarks." OFF)
+option(HIPSOLVER_ENABLE_SAMPLES "Build hipSOLVER samples." OFF)
+option(CMAKE_EXPORT_COMPILE_COMMANDS "Export compile_commands.json for clang tooling support." ON)
+


Do not declare CMAKE_EXPORT_COMPILE_COMMANDS as an option()

CMAKE_EXPORT_COMPILE_COMMANDS is a built-in CMake variable, not a project-specific option. Declaring it with option() will override the user's setting even when they explicitly pass -DCMAKE_EXPORT_COMPILE_COMMANDS=OFF on the command line. The presets already configure this variable in the base preset.

Recommended action: Remove this line. If you want to provide a default when the variable is not already defined, use:

if(NOT DEFINED CMAKE_EXPORT_COMPILE_COMMANDS) set(CMAKE_EXPORT_COMPILE_COMMANDS ON) endif()

Generated by Claude Code

I don't think that this is quite right.

ibrahimw1 · 2026-03-16T17:18:27Z

+    file(GLOB third_party_dlls
+        LIST_DIRECTORIES OFF
+        CONFIGURE_DEPENDS
+        ${cblas_DIR}/bin/*.dll
+        ${GTest_DIR}/bin/*.dll
+        $ENV{rocblas_DIR}/bin/*.dll
+        $ENV{rocsolver_DIR}/bin/*.dll
+        $ENV{HIP_DIR}/bin/*.dll
+        $ENV{HIP_DIR}/bin/hipinfo.exe
+        ${CMAKE_SOURCE_DIR}/rtest.*
    )


Avoid environment variables for dependency path resolution in file(GLOB)

This file(GLOB) block relies on environment variables ($ENV{rocblas_DIR}, $ENV{rocsolver_DIR}, $ENV{HIP_DIR}) to locate DLL files at configure time. Using environment variables for dependency paths creates fragile, non-reproducible builds that can break when the environment changes.

Additionally, line 156 uses ${CMAKE_SOURCE_DIR} which assumes this is the top-level project. This is unsafe for projects that might be consumed via add_subdirectory() and should be ${PROJECT_SOURCE_DIR} instead.

Recommended action: Resolve these paths via find_package() target properties or CMAKE_PREFIX_PATH to make the build more robust and reproducible. Change CMAKE_SOURCE_DIR to PROJECT_SOURCE_DIR for proper subdirectory support.

Generated by Claude Code

ibrahimw1 · 2026-03-16T17:18:28Z

+set_target_properties(hipsolver PROPERTIES CXX_EXTENSIONS OFF)
+
+# Warning suppression (was in hipsolver-common INTERFACE library)
+target_compile_options(hipsolver PRIVATE -Wno-unused-result)


Preserve TODO comment explaining -Wno-unused-result suppression

The comment indicates this flag was moved from the hipsolver-common INTERFACE library, but the original code included a TODO comment about addressing [[nodiscard]] warnings. Preserving that TODO ensures the warning suppression is not forgotten and signals that this is temporary technical debt.

Recommended action: Update the comment to include the TODO, for example:

# TODO: address [[nodiscard]] warnings — suppress for now target_compile_options(hipsolver PRIVATE -Wno-unused-result)

Generated by Claude Code

Don't know how I feel about this i.e. not certain it is worth the context to bother with this type of feedback. Maybe we should consider having different tiers and applying them in waves or something.

ibrahimw1 · 2026-03-16T17:18:30Z

+            "hidden": true,
+            "binaryDir": "${sourceDir}/build/${presetName}",
+            "cacheVariables": {
+                "CMAKE_CXX_COMPILER": "/opt/rocm/bin/amdclang++",


Hard-coded compiler paths reduce preset portability

The base preset hard-codes CMAKE_CXX_COMPILER and CMAKE_C_COMPILER to /opt/rocm/bin/amdclang++ and /opt/rocm/bin/amdclang. This makes the preset non-portable across environments where ROCm is installed at a different location or where developers want to use alternative compilers.

Recommended action: Consider one of these approaches:

Remove the compiler variables from the base preset and let CMAKE_PREFIX_PATH guide compiler discovery

Create a separate CMakeUserPresets.json (git-ignored) for developer-specific paths

Add a comment explaining that developers should override these in a user preset

This will make the preset more flexible and portable across different development environments.

Generated by Claude Code

ibrahimw1 · 2026-03-16T17:18:30Z

+    target_link_libraries(hipsolver-clients-common
+        PUBLIC roc::hipsolver
+        PRIVATE Threads::Threads
+    )


Clarify linking strategy for roc::hipsolver on OBJECT library

hipsolver-clients-common is an OBJECT library whose compiled objects are incorporated into the final executables (hipsolver-test, hipsolver-bench). Linking roc::hipsolver PUBLIC here means every consumer of this OBJECT library inherits roc::hipsolver transitively, which creates redundancy since downstream targets (hipsolver-bench and hipsolver-test) also explicitly link roc::hipsolver.

Recommended action: Choose one of these approaches for consistency:

Option A (preferred): Keep roc::hipsolver PUBLIC on hipsolver-clients-common and remove the redundant explicit links from hipsolver-bench and hipsolver-test

Option B: Change to PRIVATE here and keep the explicit links in the downstream targets

Option A is cleaner and reduces duplication noted in other comments.

Generated by Claude Code

ibrahimw1 · 2026-03-16T17:18:31Z

+# --- Include directories ---
+
 target_include_directories(hipsolver
-  PUBLIC  $<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/library/include>
-          $<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/library/include/internal>
-          $<BUILD_INTERFACE:${PROJECT_BINARY_DIR}/include/hipsolver>
-          $<BUILD_INTERFACE:${PROJECT_BINARY_DIR}/include/hipsolver/internal>
-          $<BUILD_INTERFACE:${PROJECT_BINARY_DIR}/include>
-          $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
-  PRIVATE
-          ${CMAKE_CURRENT_SOURCE_DIR}/include
-          ${CMAKE_CURRENT_SOURCE_DIR}
+    PUBLIC
+        $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/library/include>
+        $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/library/include/internal>


Fix PDB copy command generator expression to handle non-Debug builds

The PDB copy command on line 141 uses "$<$<CONFIG:Debug>:${CMAKE_COMMAND}>" as the COMMAND argument. When the build configuration is not Debug, this evaluates to an empty string, causing the add_custom_command to fail because COMMAND becomes invalid.

Recommended action: Restructure this to properly handle both Debug and non-Debug configurations. For example:

add_custom_command(TARGET hipsolver POST_BUILD COMMAND ${CMAKE_COMMAND} -E $<IF:$<CONFIG:Debug>,copy,true> "${PROJECT_BINARY_DIR}/staging/$<TARGET_PDB_FILE_NAME:hipsolver>" "${PROJECT_BINARY_DIR}/clients/staging/$<TARGET_PDB_FILE_NAME:hipsolver>" )

Alternatively, wrap the entire add_custom_command in a conditional block that only executes for Debug configurations. The current pattern will cause build failures in Release and other non-Debug configurations.

Generated by Claude Code

ibrahimw1 · 2026-03-16T17:18:51Z

PR Review Summary

Recommendation: Approve with suggestions

This is a well-structured modernization of the hipSOLVER CMake build system. Key improvements include:

Removing the hipsolver-common INTERFACE library and applying flags directly to targets
Adding project-prefixed options (HIPSOLVER_*) with a backward-compatibility shim
Removing the spurious project(hipsolver-clients) call in clients/
Converting clients-common from an INTERFACE library (with source files, which is not recommended) to a proper OBJECT library with PIC enabled
Using target_compile_features() instead of setting CMAKE_CXX_STANDARD globally
Removing hard-coded dependency paths from find_package() calls
Adding CMakePresets.json for standardized developer workflows
Using GNUInstallDirs and PROJECT_SOURCE_DIR instead of CMAKE_SOURCE_DIR

The main issues to address are:

Redundant transitive dependencies: roc::hipsolver and hip::host are linked both by hipsolver-clients-common (PUBLIC) and again by hipsolver-bench and hipsolver-test. Remove the redundant direct links.
Hard-coded compiler paths in CMakePresets.json reduce portability.
CMAKE_Fortran_COMPILER set in CMakeLists.txt overrides normal CMake compiler detection.
CMAKE_EXPORT_COMPILE_COMMANDS declared as option() can override user intent.
PDB copy generator expression will break on non-Debug builds.

None of these are build-breaking, but addressing them will improve the quality and maintainability of this already excellent refactoring work.

Priority	Count
High	0
Medium	8
Low	3
Total	11

Generated by Claude Code

test

a6bb4ba

github-actions Bot added the project: hipsolver label Mar 10, 2026

This comment was marked as off-topic.

Sign in to view

ibrahimw1 commented Mar 16, 2026

View reviewed changes

Conversation

ibrahimw1 commented Mar 10, 2026

Motivation

Technical Details

Root CMakeLists.txt

library/CMakeLists.txt

clients/CMakeLists.txt

CMakePresets.json

Key Improvements

Validation Results

Test Plan

Test Result

Submission Checklist

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibrahimw1 commented Mar 16, 2026

PR Review Summary

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants