Merge pull request #1369 from JackAKirk/usm-p2p-add-test-and-macro

[EXP][usm p2p] Add test for ur_exp_usm_p2p and impl for hip

Author: kbenzie

include/ur_api.h

@@ -8927,6 +8927,13 @@ urUSMReleaseExp(
 #if !defined(__GNUC__)
 #pragma region usm p2p(experimental)
 #endif
+///////////////////////////////////////////////////////////////////////////////
+#ifndef UR_USM_P2P_EXTENSION_STRING_EXP
+/// @brief The extension string that defines support for USM P2P which is
+///        returned when querying device extensions.
+#define UR_USM_P2P_EXTENSION_STRING_EXP "ur_exp_usm_p2p"
+#endif // UR_USM_P2P_EXTENSION_STRING_EXP
+
 ///////////////////////////////////////////////////////////////////////////////
 /// @brief Supported peer info
 typedef enum ur_exp_peer_info_t {

scripts/core/EXP-USM-P2P.rst

@@ -27,16 +27,20 @@ or copying the memory located on a separate "peer" device.
 
 Motivation
 --------------------------------------------------------------------------------
-Several important projects that the SYCL programming model aims to support use
-fine-grained peer to peer memory access controls.
-Two such examples that SYCL supports are Pytorch and Gromacs.
-This experimental extension to UR aims to provide a portable interface that can
-call appropriate driver functions to query and control peer memory access
-across the CUDA, HIP and L0 adapters.
+Programming models like SYCL or OpenMP aim to support several important
+projects that utilise fine-grained peer-to-peer memory access controls.
+This experimental extension to the Unified-Runtime API aims to provide a
+portable interface that can call appropriate driver functions to query and
+control peer memory access within different adapters such as CUDA, HIP and
+Level Zero.
 
 API
 --------------------------------------------------------------------------------
 
+Macros
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+* ${X}_USM_P2P_EXTENSION_STRING_EXP
+
 Enums
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
@@ -48,14 +52,23 @@ Functions
 * ${x}UsmP2PDisablePeerAccessExp
 * ${x}UsmP2PPeerAccessGetInfoExp
 
+Support
+--------------------------------------------------------------------------------
+
+Adapters which support this experimental feature *must* return the valid string
+defined in ``${X}_USM_P2P_EXTENSION_STRING_EXP`` as one of the options from
+${x}DeviceGetInfo when querying for ${X}_DEVICE_INFO_EXTENSIONS.
+
 Changelog
 --------------------------------------------------------------------------------
 
-+-----------+------------------------+
-| Revision  | Changes                |
-+===========+========================+
-| 1.0       | Initial Draft          |
-+-----------+------------------------+
++-----------+---------------------------------------------+
+| Revision  | Changes                                     |
++===========+=============================================+
+| 1.0       | Initial Draft                               |
++-----------+---------------------------------------------+
+| 1.1       | Added USM_P2P_EXTENSION_STRING_EXP ID Macro |
++-----------+---------------------------------------------+
 
 Contributors
 --------------------------------------------------------------------------------

scripts/core/exp-usm-p2p.yml

@@ -12,6 +12,11 @@ type: header
 desc: "Intel $OneApi Unified Runtime Experimental APIs for USM P2P"
 ordinal: "99"
 --- #--------------------------------------------------------------------------
+type: macro
+desc: "The extension string that defines support for USM P2P which is returned when querying device extensions."
+name: $X_USM_P2P_EXTENSION_STRING_EXP
+value: "\"$x_exp_usm_p2p\""
+--- #--------------------------------------------------------------------------
 type: enum
 desc: "Supported peer info"
 class: $xUsmP2P

source/adapters/cuda/device.cpp

@@ -612,6 +612,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urDeviceGetInfo(ur_device_handle_t hDevice,
     SupportedExtensions += "pi_ext_intel_devicelib_assert ";
     // Return supported for the UR command-buffer experimental feature
     SupportedExtensions += "ur_exp_command_buffer ";
+    SupportedExtensions += "ur_exp_usm_p2p ";
     SupportedExtensions += " ";
 
     int Major = 0;

source/adapters/hip/usm_p2p.cpp

@@ -9,25 +9,57 @@
 //===----------------------------------------------------------------------===//
 
 #include "common.hpp"
+#include "context.hpp"
 
-UR_APIEXPORT ur_result_t UR_APICALL
-urUsmP2PEnablePeerAccessExp(ur_device_handle_t, ur_device_handle_t) {
-  detail::ur::die(
-      "urUsmP2PEnablePeerAccessExp is not implemented for HIP adapter.");
-  return UR_RESULT_ERROR_UNSUPPORTED_FEATURE;
+UR_APIEXPORT ur_result_t UR_APICALL urUsmP2PEnablePeerAccessExp(
+    ur_device_handle_t commandDevice, ur_device_handle_t peerDevice) {
+  try {
+    ScopedContext active(commandDevice);
+    UR_CHECK_ERROR(hipDeviceEnablePeerAccess(peerDevice->get(), 0));
+  } catch (ur_result_t err) {
+    return err;
+  }
+  return UR_RESULT_SUCCESS;
 }
 
-UR_APIEXPORT ur_result_t UR_APICALL
-urUsmP2PDisablePeerAccessExp(ur_device_handle_t, ur_device_handle_t) {
-  detail::ur::die(
-      "urUsmP2PDisablePeerAccessExp is not implemented for HIP adapter.");
-  return UR_RESULT_ERROR_UNSUPPORTED_FEATURE;
+UR_APIEXPORT ur_result_t UR_APICALL urUsmP2PDisablePeerAccessExp(
+    ur_device_handle_t commandDevice, ur_device_handle_t peerDevice) {
+  try {
+    ScopedContext active(commandDevice);
+    UR_CHECK_ERROR(hipDeviceDisablePeerAccess(peerDevice->get()));
+  } catch (ur_result_t err) {
+    return err;
+  }
+  return UR_RESULT_SUCCESS;
 }
 
 UR_APIEXPORT ur_result_t UR_APICALL urUsmP2PPeerAccessGetInfoExp(
-    ur_device_handle_t, ur_device_handle_t, ur_exp_peer_info_t, size_t propSize,
-    void *pPropValue, size_t *pPropSizeRet) {
+    ur_device_handle_t commandDevice, ur_device_handle_t peerDevice,
+    ur_exp_peer_info_t propName, size_t propSize, void *pPropValue,
+    size_t *pPropSizeRet) {
   UrReturnHelper ReturnValue(propSize, pPropValue, pPropSizeRet);
-  // Zero return value indicates that all of the queries currently return false.
-  return ReturnValue(uint32_t{0});
+
+  int value;
+  hipDeviceP2PAttr hipAttr;
+  try {
+    ScopedContext active(commandDevice);
+    switch (propName) {
+    case UR_EXP_PEER_INFO_UR_PEER_ACCESS_SUPPORTED: {
+      hipAttr = hipDevP2PAttrAccessSupported;
+      break;
+    }
+    case UR_EXP_PEER_INFO_UR_PEER_ATOMICS_SUPPORTED: {
+      hipAttr = hipDevP2PAttrNativeAtomicSupported;
+      break;
+    }
+    default: {
+      return UR_RESULT_ERROR_INVALID_ENUMERATION;
+    }
+    }
+    UR_CHECK_ERROR(hipDeviceGetP2PAttribute(
+        &value, hipAttr, commandDevice->get(), peerDevice->get()));
+  } catch (ur_result_t err) {
+    return err;
+  }
+  return ReturnValue(value);
 }

test/conformance/CMakeLists.txt

@@ -120,6 +120,7 @@ if(UR_DPCXX)
     add_subdirectory(program)
     add_subdirectory(enqueue)
     add_subdirectory(exp_command_buffer)
+    add_subdirectory(exp_usm_p2p)
 else()
     message(WARNING
         "UR_DPCXX is not defined, the following conformance test executables \

test/conformance/exp_usm_p2p/CMakeLists.txt

@@ -0,0 +1,8 @@
+# Copyright (C) 2024 Intel Corporation
+# Part of the Unified-Runtime Project, under the Apache License v2.0 with LLVM Exceptions.
+# See LICENSE.TXT
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+add_conformance_test_with_devices_environment(exp_usm_p2p
+        usm_p2p.cpp
+  )

test/conformance/exp_usm_p2p/exp_usm_p2p_adapter_cuda.match

@@ -0,0 +1,60 @@
+// Copyright (C) 2024 Intel Corporation
+// Part of the Unified-Runtime Project, under the Apache License v2.0 with LLVM
+// Exceptions. See LICENSE.TXT
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include "uur/fixtures.h"
+#include "uur/raii.h"
+
+using urP2PTest = uur::urAllDevicesTest;
+
+TEST_F(urP2PTest, Success) {
+
+    if (devices.size() < 2) {
+        GTEST_SKIP();
+    }
+
+    size_t returned_size;
+    ASSERT_SUCCESS(urDeviceGetInfo(devices[0], UR_DEVICE_INFO_EXTENSIONS, 0,
+                                   nullptr, &returned_size));
+
+    std::unique_ptr<char[]> returned_extensions(new char[returned_size]);
+
+    ASSERT_SUCCESS(urDeviceGetInfo(devices[0], UR_DEVICE_INFO_EXTENSIONS,
+                                   returned_size, returned_extensions.get(),
+                                   nullptr));
+
+    std::string_view extensions_string(returned_extensions.get());
+    const bool usm_p2p_support =
+        extensions_string.find(UR_USM_P2P_EXTENSION_STRING_EXP) !=
+        std::string::npos;
+
+    if (!usm_p2p_support) {
+        GTEST_SKIP() << "EXP usm p2p feature is not supported.";
+    }
+
+    int value;
+    ASSERT_SUCCESS(urUsmP2PPeerAccessGetInfoExp(
+        devices[0], ///< [in] handle of the command device object
+        devices[1], ///< [in] handle of the peer device object
+        UR_EXP_PEER_INFO_UR_PEER_ACCESS_SUPPORTED, sizeof(int), &value,
+        &returned_size));
+    // Note that whilst it is not currently specified to be a requirement in the
+    // specification, currently all supported backends return value = 1 for the
+    // UR_EXP_PEER_INFO_UR_PEER_ACCESS_SUPPORTED query when the query is true
+    // (matching the native query return values). Generally different backends can
+    // return different values for a given device query; however it is
+    // advisable that for boolean queries they return the same values to indicate
+    // true/false. When this extension is moved out of experimental status such
+    // boolean return values should be specified by the extension.
+    ASSERT_EQ(value, 1);
+
+    // Just check that this doesn't throw since supporting peer atomics is
+    // optional and can depend on backend/device.
+    ASSERT_SUCCESS(urUsmP2PPeerAccessGetInfoExp(
+        devices[0], devices[1], UR_EXP_PEER_INFO_UR_PEER_ATOMICS_SUPPORTED,
+        sizeof(int), &value, &returned_size));
+
+    ASSERT_SUCCESS(urUsmP2PEnablePeerAccessExp(devices[0], devices[1]));
+    ASSERT_SUCCESS(urUsmP2PDisablePeerAccessExp(devices[0], devices[1]));
+}