Skip to content

Enhancement - Support AMDGPU_TARGETS in ROCm CMake builds #793

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
polarG wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Enhancement - Support AMDGPU_TARGETS in ROCm CMake builds #793

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions superbench/benchmarks/micro_benchmarks/rocm_common.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,25 @@ else()
set(HIP_PATH $ENV{HIP_PATH})
endif()

# Turn off CMAKE_HIP_ARCHITECTURES Feature if cmake version is 3.21+
if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.21.0)
set(CMAKE_HIP_ARCHITECTURES OFF)
# Set HIP architectures from AMDGPU_TARGETS environment variable if available.
# AMDGPU_TARGETS should be a space-separated list of GPU architectures,
# e.g. "gfx908 gfx90a gfx942".
# 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)
Comment on lines +43 to +45
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

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).

Suggested change
# 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.

Copilot uses AI. Check for mistakes.
if(DEFINED ENV{AMDGPU_TARGETS})
string(REPLACE " " ";" HIP_ARCH_LIST "$ENV{AMDGPU_TARGETS}")
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
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}")

Copilot uses AI. Check for mistakes.
set(AMDGPU_TARGETS ${HIP_ARCH_LIST} CACHE STRING "AMD GPU targets to compile for")
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

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).

Suggested change
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)

Copilot uses AI. Check for mistakes.
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}")
Comment on lines +47 to +52
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
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()

Copilot uses AI. Check for mistakes.
else()
if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.21.0)
set(CMAKE_HIP_ARCHITECTURES OFF)
endif()
message(STATUS "AMDGPU_TARGETS not set, relying on hipcc auto-detection")
endif()
message(STATUS "CMAKE HIP ARCHITECTURES: ${CMAKE_HIP_ARCHITECTURES}")

if(EXISTS ${HIP_PATH})
# Search for hip in common locations
Expand Down
Loading