Enhancement - Support AMDGPU_TARGETS in ROCm CMake builds#793
Enhancement - Support AMDGPU_TARGETS in ROCm CMake builds#793
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for driving ROCm/HIP GPU architecture selection from the AMDGPU_TARGETS environment variable during CMake configuration, to make ROCm micro-benchmark builds work on hosts where hipcc GPU auto-detection is unavailable (e.g., CI without a GPU).
Changes:
- Read
AMDGPU_TARGETSfrom the environment and convert it to a CMake list for use during configuration. - Cache
AMDGPU_TARGETSand (for CMake ≥ 3.21) setCMAKE_HIP_ARCHITECTURESwhen the env var is provided. - Preserve the prior “auto-detection” path when
AMDGPU_TARGETSis not set, and add status logging.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| string(REPLACE " " ";" HIP_ARCH_LIST "$ENV{AMDGPU_TARGETS}") | ||
| set(AMDGPU_TARGETS ${HIP_ARCH_LIST} CACHE STRING "AMD GPU targets to compile for") | ||
| if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.21.0) | ||
| set(CMAKE_HIP_ARCHITECTURES ${HIP_ARCH_LIST}) | ||
| endif() | ||
| message(STATUS "Using AMDGPU_TARGETS from environment: ${HIP_ARCH_LIST}") |
There was a problem hiding this comment.
if(DEFINED ENV{AMDGPU_TARGETS}) is true even when the environment variable exists but is empty. In that case HIP_ARCH_LIST becomes empty and this will cache an empty AMDGPU_TARGETS, potentially breaking the HIP toolchain configuration instead of falling back to auto-detection. Consider checking that $ENV{AMDGPU_TARGETS} is non-empty (after trimming) before taking this branch.
| string(REPLACE " " ";" HIP_ARCH_LIST "$ENV{AMDGPU_TARGETS}") | |
| set(AMDGPU_TARGETS ${HIP_ARCH_LIST} CACHE STRING "AMD GPU targets to compile for") | |
| if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.21.0) | |
| set(CMAKE_HIP_ARCHITECTURES ${HIP_ARCH_LIST}) | |
| endif() | |
| message(STATUS "Using AMDGPU_TARGETS from environment: ${HIP_ARCH_LIST}") | |
| # Copy and trim the environment variable to ensure it is non-empty after stripping whitespace. | |
| set(_amdgpu_targets_raw "$ENV{AMDGPU_TARGETS}") | |
| string(STRIP "${_amdgpu_targets_raw}" _amdgpu_targets) | |
| if(NOT _amdgpu_targets STREQUAL "") | |
| string(REPLACE " " ";" HIP_ARCH_LIST "${_amdgpu_targets}") | |
| set(AMDGPU_TARGETS ${HIP_ARCH_LIST} CACHE STRING "AMD GPU targets to compile for") | |
| if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.21.0) | |
| set(CMAKE_HIP_ARCHITECTURES ${HIP_ARCH_LIST}) | |
| endif() | |
| message(STATUS "Using AMDGPU_TARGETS from environment: ${HIP_ARCH_LIST}") | |
| else() | |
| if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.21.0) | |
| set(CMAKE_HIP_ARCHITECTURES OFF) | |
| endif() | |
| message(STATUS "AMDGPU_TARGETS is defined but empty, relying on hipcc auto-detection") | |
| endif() |
| # - CMAKE_HIP_ARCHITECTURES is the native CMake variable for HIP (requires >= 3.21) | ||
| if(DEFINED ENV{AMDGPU_TARGETS}) | ||
| string(REPLACE " " ";" HIP_ARCH_LIST "$ENV{AMDGPU_TARGETS}") | ||
| set(AMDGPU_TARGETS ${HIP_ARCH_LIST} CACHE STRING "AMD GPU targets to compile for") |
There was a problem hiding this comment.
set(AMDGPU_TARGETS ... CACHE STRING ...) will not override an existing cached AMDGPU_TARGETS value. If a build directory is reconfigured with a different AMDGPU_TARGETS environment variable, this code may silently keep using the old cached value. Decide the intended precedence (env vs cache) and implement it explicitly (e.g., only set when the cache entry is unset, or use FORCE when env should win).
| set(AMDGPU_TARGETS ${HIP_ARCH_LIST} CACHE STRING "AMD GPU targets to compile for") | |
| set(AMDGPU_TARGETS ${HIP_ARCH_LIST} CACHE STRING "AMD GPU targets to compile for" FORCE) |
| # Both the CMake variable AMDGPU_TARGETS and CMAKE_HIP_ARCHITECTURES must be set: | ||
| # - AMDGPU_TARGETS is read by ROCm's hip-config-amd.cmake to set --offload-arch flags | ||
| # - CMAKE_HIP_ARCHITECTURES is the native CMake variable for HIP (requires >= 3.21) |
There was a problem hiding this comment.
The comment says both AMDGPU_TARGETS and CMAKE_HIP_ARCHITECTURES must be set, but the micro-benchmark CMake projects in this repo are LANGUAGES CXX and compile via hipcc as the C++ compiler (they don’t enable the HIP language). In that setup, CMAKE_HIP_ARCHITECTURES is unlikely to be consumed by CMake at all, so this note can be misleading. Please adjust the comment (and/or the logic) to reflect what actually drives offload arch selection here (typically AMDGPU_TARGETS as read by hip-config-amd.cmake).
| # Both the CMake variable AMDGPU_TARGETS and CMAKE_HIP_ARCHITECTURES must be set: | |
| # - AMDGPU_TARGETS is read by ROCm's hip-config-amd.cmake to set --offload-arch flags | |
| # - CMAKE_HIP_ARCHITECTURES is the native CMake variable for HIP (requires >= 3.21) | |
| # In this repository's micro-benchmarks, AMDGPU_TARGETS is what actually drives | |
| # --offload-arch selection, via ROCm's hip-config-amd.cmake and hipcc. | |
| # CMAKE_HIP_ARCHITECTURES is set (when supported by CMake >= 3.21) for compatibility | |
| # with projects that enable the CMake HIP language, but is not required here. |
| # - AMDGPU_TARGETS is read by ROCm's hip-config-amd.cmake to set --offload-arch flags | ||
| # - CMAKE_HIP_ARCHITECTURES is the native CMake variable for HIP (requires >= 3.21) | ||
| if(DEFINED ENV{AMDGPU_TARGETS}) | ||
| string(REPLACE " " ";" HIP_ARCH_LIST "$ENV{AMDGPU_TARGETS}") |
There was a problem hiding this comment.
string(REPLACE " " ";" ...) will turn repeated spaces (or leading/trailing spaces) into empty list elements (e.g., gfx90a gfx942 -> gfx90a;;gfx942). That can propagate into AMDGPU_TARGETS/CMAKE_HIP_ARCHITECTURES and potentially generate invalid arch flags. Consider normalizing whitespace first (e.g., regex replace [ \t]+ with ; and stripping) or using a list-splitting helper that avoids empty entries.
| string(REPLACE " " ";" HIP_ARCH_LIST "$ENV{AMDGPU_TARGETS}") | |
| # Normalize whitespace to avoid empty list elements: | |
| # 1) strip leading/trailing whitespace | |
| # 2) collapse runs of spaces/tabs into a single list separator ';' | |
| set(_AMDGPU_TARGETS_RAW "$ENV{AMDGPU_TARGETS}") | |
| string(STRIP "${_AMDGPU_TARGETS_RAW}" _AMDGPU_TARGETS_STRIPPED) | |
| string(REGEX REPLACE "[ \t]+" ";" HIP_ARCH_LIST "${_AMDGPU_TARGETS_STRIPPED}") |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #793 +/- ##
=======================================
Coverage 85.69% 85.69%
=======================================
Files 103 103
Lines 7890 7890
=======================================
Hits 6761 6761
Misses 1129 1129
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Allow specifying AMD GPU architectures via the AMDGPU_TARGETS environment variable in rocm_common.cmake. This fixes builds on machines without a GPU (e.g., CI) where hipcc auto-detection fails. When unset, the previous auto-detection behavior is preserved.