Skip to content

[offload] [test] Skip amdgcn/nvptx tests if detected arch is not built #119006

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.

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

Closed
wants to merge 2 commits into from

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