[OpenMP] Add unit tests for nextgen plugins #74398

EthanLuisMcDonough · 2023-12-05T01:23:23Z

This patch add three GTest unit tests that test plugin read and write operations. Tests can be compiled with ninja -C runtimes/runtimes-bins LibomptUnitTests.

llvmbot · 2023-12-05T01:23:44Z

@llvm/pr-subscribers-openmp

Author: Ethan Luis McDonough (EthanLuisMcDonough)

Changes

This patch add three GTest unit tests that test plugin read and write operations. Tests can be compiled with ninja -C runtimes/runtimes-bins LibomptUnitTests.

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

4 Files Affected:

(modified) openmp/libomptarget/CMakeLists.txt (+6)
(added) openmp/libomptarget/unittests/CMakeLists.txt (+8)
(added) openmp/libomptarget/unittests/Plugins/CMakeLists.txt (+26)
(added) openmp/libomptarget/unittests/Plugins/NextgenPluginsTest.cpp (+146)

diff --git a/openmp/libomptarget/CMakeLists.txt b/openmp/libomptarget/CMakeLists.txt
index 5f592f46c8ffa..6a474469ab87a 100644
--- a/openmp/libomptarget/CMakeLists.txt
+++ b/openmp/libomptarget/CMakeLists.txt
@@ -123,3 +123,9 @@ add_subdirectory(tools)
 
 # Add tests.
 add_subdirectory(test)
+
+# Add unit tests if GMock/GTest is present
+if (EXISTS ${LLVM_THIRD_PARTY_DIR}/unittest)
+  add_subdirectory(${LLVM_THIRD_PARTY_DIR}/unittest ${CMAKE_CURRENT_BINARY_DIR}/third-party/unittest)
+  add_subdirectory(unittests)
+endif()
diff --git a/openmp/libomptarget/unittests/CMakeLists.txt b/openmp/libomptarget/unittests/CMakeLists.txt
new file mode 100644
index 0000000000000..0a29afd68569a
--- /dev/null
+++ b/openmp/libomptarget/unittests/CMakeLists.txt
@@ -0,0 +1,8 @@
+add_custom_target(LibomptUnitTests)
+set_target_properties(LibomptUnitTests PROPERTIES FOLDER "Libomptarget tests")
+
+function(add_libompt_unittest test_dirname)
+  add_unittest(LibomptUnitTests ${test_dirname} ${ARGN})
+endfunction()
+
+add_subdirectory(Plugins)
diff --git a/openmp/libomptarget/unittests/Plugins/CMakeLists.txt b/openmp/libomptarget/unittests/Plugins/CMakeLists.txt
new file mode 100644
index 0000000000000..234ba699c4834
--- /dev/null
+++ b/openmp/libomptarget/unittests/Plugins/CMakeLists.txt
@@ -0,0 +1,26 @@
+set(LIBOMPTARGET_BUILD_AMDGPU_PLUGIN TRUE CACHE BOOL
+  "Whether to build AMDGPU plugin")
+set(LIBOMPTARGET_BUILD_CUDA_PLUGIN TRUE CACHE BOOL
+  "Whether to build CUDA plugin")
+
+set(PLUGINS_TEST_COMMON omptarget OMPT omptarget.devicertl)
+set(PLUGINS_TEST_SOURCES NextgenPluginsTest.cpp)
+set(PLUGINS_TEST_INCLUDE ${LIBOMPTARGET_INCLUDE_DIR})
+
+if (LIBOMPTARGET_BUILD_AMDGPU_PLUGIN)
+  libomptarget_say("Building plugin unit tests for AMDGPU")
+  add_libompt_unittest(PluginsTestAmd ${PLUGINS_TEST_SOURCES})
+  add_dependencies(PluginsTestAmd ${PLUGINS_TEST_COMMON} omptarget.rtl.amdgpu)
+  target_link_libraries(PluginsTestAmd PRIVATE ${PLUGINS_TEST_COMMON} omptarget.rtl.amdgpu)
+  target_include_directories(PluginsTestAmd PRIVATE ${PLUGINS_TEST_INCLUDE})
+else()
+  libomptarget_say("Skipping AMDGPU plugin unit tests")
+endif()
+
+if (LIBOMPTARGET_BUILD_CUDA_PLUGIN)
+  libomptarget_say("Building plugin unit tests for Cuda")
+  add_libompt_unittest(PluginsTestCu ${PLUGINS_TEST_SOURCES})
+  add_dependencies(PluginsTestCu ${PLUGINS_TEST_COMMON} omptarget.rtl.cuda)
+  target_link_libraries(PluginsTestCu PRIVATE ${PLUGINS_TEST_COMMON} omptarget.rtl.cuda)
+  target_include_directories(PluginsTestCu PRIVATE ${PLUGINS_TEST_INCLUDE})
+endif()
diff --git a/openmp/libomptarget/unittests/Plugins/NextgenPluginsTest.cpp b/openmp/libomptarget/unittests/Plugins/NextgenPluginsTest.cpp
new file mode 100644
index 0000000000000..24311076e8acd
--- /dev/null
+++ b/openmp/libomptarget/unittests/Plugins/NextgenPluginsTest.cpp
@@ -0,0 +1,146 @@
+#include "Shared/PluginAPI.h"
+#include "omptarget.h"
+#include "gtest/gtest.h"
+
+const int DEVICE_ID = 0, DEVICE_TWO = 1;
+bool setup_map[DEVICE_TWO + 1];
+
+int init_test_device(int ID) {
+  if (setup_map[ID]) {
+    return OFFLOAD_SUCCESS;
+  }
+  if (__tgt_rtl_init_plugin() == OFFLOAD_FAIL ||
+      __tgt_rtl_init_device(ID) == OFFLOAD_FAIL) {
+    return OFFLOAD_FAIL;
+  }
+  setup_map[ID] = true;
+  return OFFLOAD_SUCCESS;
+}
+
+// Test plugin initialization
+TEST(NextgenPluginsTest, PluginInit) {
+  EXPECT_EQ(OFFLOAD_SUCCESS, init_test_device(DEVICE_ID));
+}
+
+// Test GPU allocation and R/W
+TEST(NextgenPluginsTest, PluginAlloc) {
+  int32_t test_value = 23;
+  int32_t host_value = -1;
+  int64_t var_size = sizeof(int32_t);
+
+  // Init plugin and device
+  EXPECT_EQ(OFFLOAD_SUCCESS, init_test_device(DEVICE_ID));
+
+  // Allocate memory
+  void *device_ptr =
+      __tgt_rtl_data_alloc(DEVICE_ID, var_size, nullptr, TARGET_ALLOC_DEFAULT);
+
+  // Check that the result is not null
+  EXPECT_NE(device_ptr, nullptr);
+
+  // Submit data to device
+  EXPECT_EQ(OFFLOAD_SUCCESS, __tgt_rtl_data_submit(DEVICE_ID, device_ptr,
+                                                   &test_value, var_size));
+
+  // Read data from device
+  EXPECT_EQ(OFFLOAD_SUCCESS, __tgt_rtl_data_retrieve(DEVICE_ID, &host_value,
+                                                     device_ptr, var_size));
+
+  // Compare values
+  EXPECT_EQ(host_value, test_value);
+
+  // Cleanup data
+  EXPECT_EQ(OFFLOAD_SUCCESS,
+            __tgt_rtl_data_delete(DEVICE_ID, device_ptr, TARGET_ALLOC_DEFAULT));
+}
+
+// Test async GPU allocation and R/W
+TEST(NextgenPluginsTest, PluginAsyncAlloc) {
+  int32_t test_value = 47;
+  int32_t host_value = -1;
+  int64_t var_size = sizeof(int32_t);
+  __tgt_async_info info;
+
+  // Init plugin and device
+  EXPECT_EQ(OFFLOAD_SUCCESS, init_test_device(DEVICE_ID));
+
+  // Allocate memory
+  void *device_ptr =
+      __tgt_rtl_data_alloc(DEVICE_ID, var_size, nullptr, TARGET_ALLOC_DEFAULT);
+
+  // Check that the result is not null
+  EXPECT_NE(device_ptr, nullptr);
+
+  // Submit data to device asynchronously
+  EXPECT_EQ(OFFLOAD_SUCCESS,
+            __tgt_rtl_data_submit_async(DEVICE_ID, device_ptr, &test_value,
+                                        var_size, &info));
+
+  // Wait for async request to process
+  EXPECT_EQ(OFFLOAD_SUCCESS, __tgt_rtl_synchronize(DEVICE_ID, &info));
+
+  // Read data from device
+  EXPECT_EQ(OFFLOAD_SUCCESS,
+            __tgt_rtl_data_retrieve_async(DEVICE_ID, &host_value, device_ptr,
+                                          var_size, &info));
+
+  // Wait for async request to process
+  EXPECT_EQ(OFFLOAD_SUCCESS, __tgt_rtl_synchronize(DEVICE_ID, &info));
+
+  // Compare values
+  EXPECT_EQ(host_value, test_value);
+
+  // Cleanup data
+  EXPECT_EQ(OFFLOAD_SUCCESS,
+            __tgt_rtl_data_delete(DEVICE_ID, device_ptr, TARGET_ALLOC_DEFAULT));
+}
+
+// Test GPU data exchange
+TEST(NextgenPluginsTest, PluginDataSwap) {
+  int32_t test_value = 23;
+  int32_t host_value = -1;
+  int64_t var_size = sizeof(int32_t);
+
+  // Only run test if we have multiple GPUs to test
+  // GPUs must be compatible for test to work
+  if (__tgt_rtl_number_of_devices() > 1 &&
+      __tgt_rtl_is_data_exchangable(DEVICE_ID, DEVICE_TWO)) {
+    // Init both GPUs
+    EXPECT_EQ(OFFLOAD_SUCCESS, init_test_device(DEVICE_ID));
+    EXPECT_EQ(OFFLOAD_SUCCESS, init_test_device(DEVICE_TWO));
+
+    // Allocate memory on both GPUs
+    // DEVICE_ID will be the source
+    // DEVICE_TWO will be the destination
+    void *source_ptr = __tgt_rtl_data_alloc(DEVICE_ID, var_size, nullptr,
+                                            TARGET_ALLOC_DEFAULT);
+    void *dest_ptr = __tgt_rtl_data_alloc(DEVICE_TWO, var_size, nullptr,
+                                          TARGET_ALLOC_DEFAULT);
+
+    // Check for success in allocation
+    EXPECT_NE(source_ptr, nullptr);
+    EXPECT_NE(dest_ptr, nullptr);
+
+    // Write data to source
+    EXPECT_EQ(OFFLOAD_SUCCESS, __tgt_rtl_data_submit(DEVICE_ID, source_ptr,
+                                                     &test_value, var_size));
+
+    // Transfer data between devices
+    EXPECT_EQ(OFFLOAD_SUCCESS,
+              __tgt_rtl_data_exchange(DEVICE_ID, source_ptr, DEVICE_TWO,
+                                      dest_ptr, var_size));
+
+    // Read from destination device (DEVICE_TWO) memory
+    EXPECT_EQ(OFFLOAD_SUCCESS, __tgt_rtl_data_retrieve(DEVICE_TWO, &host_value,
+                                                       dest_ptr, var_size));
+
+    // Ensure match
+    EXPECT_EQ(host_value, test_value);
+
+    // Cleanup
+    EXPECT_EQ(OFFLOAD_SUCCESS, __tgt_rtl_data_delete(DEVICE_ID, source_ptr,
+                                                     TARGET_ALLOC_DEFAULT));
+    EXPECT_EQ(OFFLOAD_SUCCESS, __tgt_rtl_data_delete(DEVICE_TWO, dest_ptr,
+                                                     TARGET_ALLOC_DEFAULT));
+  }
+}

jhuber6

Thanks, I've been wanting the ability to test stuff without relying on OpenMP codegen for awhile.

jhuber6 · 2023-12-05T01:29:18Z

openmp/libomptarget/unittests/CMakeLists.txt

@@ -0,0 +1,8 @@
+add_custom_target(LibomptUnitTests)
+set_target_properties(LibomptUnitTests PROPERTIES FOLDER "Libomptarget tests")


Suggested change

set_target_properties(LibomptUnitTests PROPERTIES FOLDER "Libomptarget tests")

set_target_properties(LibomptUnitTests PROPERTIES FOLDER "Tests/UnitTests")

This is a weird folder name.

jhuber6 · 2023-12-05T01:34:17Z

openmp/libomptarget/unittests/Plugins/CMakeLists.txt

+set(PLUGINS_TEST_SOURCES NextgenPluginsTest.cpp)
+set(PLUGINS_TEST_INCLUDE ${LIBOMPTARGET_INCLUDE_DIR})
+
+if (LIBOMPTARGET_BUILD_AMDGPU_PLUGIN)


Suggested change

if (LIBOMPTARGET_BUILD_AMDGPU_PLUGIN)

if(TARGET omptarget.rtl.amdgpu)

We should be able to just check the existence of the target so long as we add the unit tests after the main source.

jhuber6 · 2023-12-05T01:35:00Z

openmp/libomptarget/unittests/Plugins/CMakeLists.txt

+set(LIBOMPTARGET_BUILD_AMDGPU_PLUGIN TRUE CACHE BOOL
+  "Whether to build AMDGPU plugin")
+set(LIBOMPTARGET_BUILD_CUDA_PLUGIN TRUE CACHE BOOL
+  "Whether to build CUDA plugin")
+


Suggested change

set(LIBOMPTARGET_BUILD_AMDGPU_PLUGIN TRUE CACHE BOOL

"Whether to build AMDGPU plugin")

set(LIBOMPTARGET_BUILD_CUDA_PLUGIN TRUE CACHE BOOL

"Whether to build CUDA plugin")

Aren't these already declared somewhere else?

jhuber6 · 2023-12-05T01:36:41Z

openmp/libomptarget/unittests/Plugins/CMakeLists.txt

+  add_dependencies(PluginsTestCu ${PLUGINS_TEST_COMMON} omptarget.rtl.cuda)
+  target_link_libraries(PluginsTestCu PRIVATE ${PLUGINS_TEST_COMMON} omptarget.rtl.cuda)
+  target_include_directories(PluginsTestCu PRIVATE ${PLUGINS_TEST_INCLUDE})
+endif()


Realistically this should be a loop over all the targets, just have the plugin append some CMake list of enabled targets, then we can just iterate it here.

jdoerfert

Thanks, I think after Joseph's comments are addressed and the licence is in, we can merge this.

openmp/libomptarget/unittests/Plugins/NextgenPluginsTest.cpp

jdoerfert · 2023-12-05T15:14:56Z

openmp/libomptarget/unittests/Plugins/NextgenPluginsTest.cpp

+#include "omptarget.h"
+#include "gtest/gtest.h"
+
+const int DEVICE_ID = 0, DEVICE_TWO = 1;


In a follow up we should iterate over all devices, not hardcode 2.

jplehr · 2023-12-07T10:09:41Z

openmp/libomptarget/unittests/Plugins/NextgenPluginsTest.cpp

+}
+
+// Test plugin initialization
+TEST(NextgenPluginsTest, PluginInit) {


Is there a reason for not using a test fixture for this instead of the global variables?

The global variables serve to keep track of which devices have been initialized. Devices can't be re-initialized, so I think the initialized devices would have to either be tracked through static class members or exist outside the test fixture to persist across tests. I don't think there's a way to check if a device has been initialized through the functions defined in PluginAPI.h, but I definitely could be overlooking something.

That makes sense, thanks for explaining.

github-actions · 2023-12-15T08:06:09Z

:white_check_mark: With the latest revision this PR passed the C/C++ code formatter.

jdoerfert

I'm fine with getting this in and adapting it in-tree.

@jplehr concerns?

jdoerfert · 2023-12-19T23:41:47Z

openmp/libomptarget/CMakeLists.txt

+if (EXISTS ${LLVM_THIRD_PARTY_DIR}/unittest)
+  add_subdirectory(${LLVM_THIRD_PARTY_DIR}/unittest ${CMAKE_CURRENT_BINARY_DIR}/third-party/unittest)
+  add_subdirectory(unittests)
+endif()


lld just does this:
llvm_add_unittests(LLD_UNITTESTS_ADDED)
See

llvm-project/lld/CMakeLists.txt

Line 195 in 3942749

llvm_add_unittests(LLD_UNITTESTS_ADDED)

and

https://github.com/llvm/llvm-project/blob/394274965a119973612c25e0eaf299c8954cce94/llvm/cmake/modules/AddLLVM.cmake#L2497C2-L2497C2

We should check if we can do the same, reducing the logic we carry.

Pretty sure that's a name defined in the same way, we could use the same name if needed.

We also need these to run w/ check-openmp or similar. Long term I'd like language tests to be all all in a different directory so we can test things like CUDA, HIP, SYCL, OpenACC, C, C++, Fortran, etc.

jplehr · 2023-12-20T08:26:09Z

I'm fine with getting this in and adapting it in-tree.

@jplehr concerns?

No, I think that makes sense.

vzakhari · 2023-12-21T01:06:39Z

Hi @EthanLuisMcDonough, @jdoerfert, do you know what might be causing this buildbot failure https://lab.llvm.org/buildbot/#/builders/270/builds/5927/steps/6/logs/stdio?

vvereschaka · 2023-12-21T06:06:22Z

looks like these changes break the llvm configuration when openmp is a part of LLVM_ENABLE_PROJECTS, but not in LLVM_ENABLE_RUNTIMES. It tries to create new build targets with already existing names.

  add_library cannot create target "llvm_gtest" because another target with
  the same name already exists.  The existing target is a static library
  created in source directory

and

 add_library cannot create target "llvm_gtest_main" because another target
 with the same name already exists.  The existing target is a static library
 created in source directory

https://lab.llvm.org/buildbot/#/builders/270/builds/5927/steps/6/logs/stdio

This patch addresses an issue introduced in pull request #74398. CMake will attempt to re-build gtest if openmp is enabled as a project (as opposed to being enabled as a runtime). This patch adds a check that prevents this from happening.

EthanLuisMcDonough · 2023-12-21T10:11:01Z

I think #76141 should resolve this. I've my patch out locally, and it seems to fix the issue. I hope you all don't mind me immediately merging it; I didn't want to keep the buildbot failing.

vvereschaka · 2023-12-21T20:08:20Z

Thank you @EthanLuisMcDonough

EthanLuisMcDonough added 2 commits December 2, 2023 02:23

Add unit testing to libomptarget

2bb2d4b

Add tests

148f8c9

EthanLuisMcDonough added openmp openmp:libomp OpenMP host runtime labels Dec 5, 2023

EthanLuisMcDonough requested a review from jdoerfert December 5, 2023 01:23

llvmbot added the openmp:libomptarget OpenMP offload runtime label Dec 5, 2023

jhuber6 reviewed Dec 5, 2023

View reviewed changes

jdoerfert reviewed Dec 5, 2023

View reviewed changes

jplehr reviewed Dec 7, 2023

View reviewed changes

Refactor test CMake + second device location

d116416

Fix clang formatting

4a05803

EthanLuisMcDonough requested review from jdoerfert and jhuber6 December 15, 2023 08:57

EthanLuisMcDonough changed the title ~~[WIP] [OpenMP] Add unit tests for nextgen plugins~~ [OpenMP] Add unit tests for nextgen plugins Dec 15, 2023

jdoerfert approved these changes Dec 20, 2023

View reviewed changes

jdoerfert merged commit 3c10e5b into llvm:main Dec 20, 2023

EthanLuisMcDonough deleted the ethanmc branch December 21, 2023 09:51

EthanLuisMcDonough mentioned this pull request Dec 21, 2023

[OpenMP] Check for gtest when building libomptarget unit tests #76141

Merged

		@@ -0,0 +1,8 @@
		add_custom_target(LibomptUnitTests)
		set_target_properties(LibomptUnitTests PROPERTIES FOLDER "Libomptarget tests")

	if (LIBOMPTARGET_BUILD_AMDGPU_PLUGIN)
	if(TARGET omptarget.rtl.amdgpu)

[OpenMP] Add unit tests for nextgen plugins #74398

[OpenMP] Add unit tests for nextgen plugins #74398

Conversation

EthanLuisMcDonough commented Dec 5, 2023

Uh oh!

llvmbot commented Dec 5, 2023

Uh oh!

jhuber6 left a comment

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!

jdoerfert left a comment

Choose a reason for hiding this comment

Uh oh!

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!

github-actions bot commented Dec 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdoerfert left a comment

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!

jplehr commented Dec 20, 2023

Uh oh!

vzakhari commented Dec 21, 2023

Uh oh!

vvereschaka commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EthanLuisMcDonough commented Dec 21, 2023

Uh oh!

vvereschaka commented Dec 21, 2023

Uh oh!

Uh oh!

github-actions bot commented Dec 15, 2023 •

edited

Loading

vvereschaka commented Dec 21, 2023 •

edited

Loading