Skip to content

Conversation

@mgorny
Copy link
Member

@mgorny mgorny commented Dec 6, 2024

Skip amdgcn and/or nvptx tests, if the detected GPU architecture is not present among GPUs offload was built for. Without this change, the tests are run if any GPU is detected -- which could lead to cryptic test failures, such as the ones reported in #118824.

Skip amdgcn and/or nvptx tests, if the detected GPU architecture
is not present among GPUs offload was built for.  Without this change,
the tests are run if any GPU is detected -- which could lead
to cryptic test failures, such as the ones reported in llvm#118824.
@mgorny mgorny requested a review from jhuber6 December 6, 2024 18:25
@llvmbot llvmbot added the offload label Dec 6, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 6, 2024

@llvm/pr-subscribers-backend-amdgpu

@llvm/pr-subscribers-offload

Author: Michał Górny (mgorny)

Changes

Skip amdgcn and/or nvptx tests, if the detected GPU architecture is not present among GPUs offload was built for. Without this change, the tests are run if any GPU is detected -- which could lead to cryptic test failures, such as the ones reported in #118824.


Full diff: https://github.com/llvm/llvm-project/pull/119006.diff

2 Files Affected:

  • (modified) offload/DeviceRTL/CMakeLists.txt (+3)
  • (modified) offload/test/CMakeLists.txt (+14)
diff --git a/offload/DeviceRTL/CMakeLists.txt b/offload/DeviceRTL/CMakeLists.txt
index 32a7510be980d8..94746b35ea7e0d 100644
--- a/offload/DeviceRTL/CMakeLists.txt
+++ b/offload/DeviceRTL/CMakeLists.txt
@@ -69,6 +69,9 @@ elseif(LIBOMPTARGET_DEVICE_ARCHITECTURES STREQUAL "auto" OR
       "${LIBOMPTARGET_NVPTX_DETECTED_ARCH_LIST};${LIBOMPTARGET_AMDGPU_DETECTED_ARCH_LIST}")
 endif()
 list(REMOVE_DUPLICATES LIBOMPTARGET_DEVICE_ARCHITECTURES)
+# for tests
+set(LIBOMPTARGET_EXPANDED_DEVICE_ARCHITECTURES ${LIBOMPTARGET_DEVICE_ARCHITECTURES}
+    PARENT_SCOPE)
 
 set(include_files
   ${include_directory}/Allocator.h
diff --git a/offload/test/CMakeLists.txt b/offload/test/CMakeLists.txt
index 8a827e0a625eff..9ab213acd23be2 100644
--- a/offload/test/CMakeLists.txt
+++ b/offload/test/CMakeLists.txt
@@ -37,6 +37,20 @@ string(REGEX MATCHALL "([^\ ]+\ |[^\ ]+$)" SYSTEM_TARGETS "${LIBOMPTARGET_SYSTEM
 foreach(CURRENT_TARGET IN LISTS SYSTEM_TARGETS)
   string(STRIP "${CURRENT_TARGET}" CURRENT_TARGET)
 
+  if(CURRENT_TARGET MATCHES "^amdgcn" AND
+     NOT "${LIBOMPTARGET_AMDGPU_DETECTED_ARCH_LIST}"
+         IN_LIST LIBOMPTARGET_EXPANDED_DEVICE_ARCHITECTURES)
+    message(WARNING "Detected AMDGPU arch ${LIBOMPTARGET_AMDGPU_DETECTED_ARCH_LIST} "
+                    "not in built arch list, ${CURRENT_TARGET} tests will be skipped")
+    continue()
+  elseif(CURRENT_TARGET MATCHES "^nvptx" AND
+     NOT "${LIBOMPTARGET_DEP_CUDA_ARCH}"
+         IN_LIST LIBOMPTARGET_EXPANDED_DEVICE_ARCHITECTURES)
+    message(WARNING "Detected NVPTX arch ${LIBOMPTARGET_DEP_CUDA_ARCH} "
+                    "not in built arch list, ${CURRENT_TARGET} tests will be skipped")
+    continue()
+  endif()
+
   add_offload_testsuite(check-libomptarget-${CURRENT_TARGET}
     "Running libomptarget tests"
     ${CMAKE_CURRENT_BINARY_DIR}/${CURRENT_TARGET}

Copy link
Contributor

@shiltian shiltian left a comment

Choose a reason for hiding this comment

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

A potentially better alternative would be to define a CMake variable when a target is enabled, and then check if the variable exists and is true when enabling tests.

@mgorny
Copy link
Member Author

mgorny commented Dec 7, 2024

A potentially better alternative would be to define a CMake variable when a target is enabled, and then check if the variable exists and is true when enabling tests.

You mean in plugins-nextgen/*/CMakeLists.txt? I suppose I could even reuse the existing logic there.

@mgorny
Copy link
Member Author

mgorny commented Dec 7, 2024

Hmm, except that plugins-nextgen is built before DeviceRTL, so we don't have the list there yet. No clue if they can be safely reordered.

@jhuber6
Copy link
Contributor

jhuber6 commented Dec 7, 2024

Hmm, except that plugins-nextgen is built before DeviceRTL, so we don't have the list there yet. No clue if they can be safely reordered.

Don't think they depend on eachother, they used to a long time ago.

@mgorny
Copy link
Member Author

mgorny commented Dec 7, 2024

Does this version look better? Or should I move the condition straight into DeviceRTL and just pass two booleans from there?

Copy link
Contributor

@shiltian shiltian left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants