From e75f13b38c5591912e42b3b4b5cfd350f51239c3 Mon Sep 17 00:00:00 2001 From: Diptorup Deb Date: Fri, 3 Feb 2023 20:08:22 -0600 Subject: [PATCH] Changes the API for DPCTLUSM_GetPointerType. - DPCTLUSM_GetPointerType used to return a global string literal and had an API that is different from all other libsyclinterface functions that return a C string. Returning a pointer to a global literal as opposed to a heap allocated memory meant the ownership of the pointer was not clear when crossing language boundary. Calling DPCTLCString_Delete on the previously returned const char* will cause a segfault - The PR introduces an enum for usm types and returns an enum value from DPCTLUSM_GetPointerType. - All Python API is unaffected. --- dpctl/_backend.pxd | 11 ++++++-- dpctl/memory/_memory.pyx | 27 ++++++++++++------- .../include/dpctl_sycl_enum_types.h | 12 +++++++++ .../include/dpctl_sycl_usm_interface.h | 10 ++++--- .../source/dpctl_sycl_usm_interface.cpp | 17 ++++++------ .../tests/test_sycl_usm_interface.cpp | 26 ++++++++---------- 6 files changed, 64 insertions(+), 39 deletions(-) diff --git a/dpctl/_backend.pxd b/dpctl/_backend.pxd index 42406d8b9d..23200cd1e5 100644 --- a/dpctl/_backend.pxd +++ b/dpctl/_backend.pxd @@ -34,6 +34,12 @@ cdef extern from "syclinterface/dpctl_utils.h": cdef extern from "syclinterface/dpctl_sycl_enum_types.h": + ctypedef enum _usm_type 'DPCTLSyclUSMType': + _USM_UNKNOWN 'DPCTL_USM_UNKNOWN' + _USM_DEVICE 'DPCTL_USM_DEVICE' + _USM_HOST 'DPCTL_USM_HOST' + _USM_SHARED 'DPCTL_USM_SHARED' + ctypedef enum _backend_type 'DPCTLSyclBackendType': _ALL_BACKENDS 'DPCTL_ALL_BACKENDS' _CUDA 'DPCTL_CUDA' @@ -340,7 +346,8 @@ cdef extern from "syclinterface/dpctl_sycl_kernel_bundle_interface.h": cdef bool DPCTLKernelBundle_HasKernel(DPCTLSyclKernelBundleRef KBRef, const char *KernelName) cdef void DPCTLKernelBundle_Delete(DPCTLSyclKernelBundleRef KBRef) - cdef DPCTLSyclKernelBundleRef DPCTLKernelBundle_Copy(const DPCTLSyclKernelBundleRef KBRef) + cdef DPCTLSyclKernelBundleRef DPCTLKernelBundle_Copy( + const DPCTLSyclKernelBundleRef KBRef) cdef extern from "syclinterface/dpctl_sycl_queue_interface.h": @@ -450,7 +457,7 @@ cdef extern from "syclinterface/dpctl_sycl_usm_interface.h": cdef void DPCTLfree_with_context( DPCTLSyclUSMRef MRef, DPCTLSyclContextRef CRef) - cdef const char* DPCTLUSM_GetPointerType( + cdef _usm_type DPCTLUSM_GetPointerType( DPCTLSyclUSMRef MRef, DPCTLSyclContextRef CRef) cdef DPCTLSyclDeviceRef DPCTLUSM_GetPointerDevice( diff --git a/dpctl/memory/_memory.pyx b/dpctl/memory/_memory.pyx index b114fc2326..93d5b507e0 100644 --- a/dpctl/memory/_memory.pyx +++ b/dpctl/memory/_memory.pyx @@ -55,6 +55,7 @@ from dpctl._backend cimport ( # noqa: E211 DPCTLSyclUSMRef, DPCTLUSM_GetPointerDevice, DPCTLUSM_GetPointerType, + _usm_type, ) from .._sycl_context cimport SyclContext @@ -232,10 +233,10 @@ cdef class _Memory: cdef _getbuffer(self, Py_buffer *buffer, int flags): # memory_ptr is Ref which is pointer to SYCL type. For USM it is void*. cdef SyclContext ctx = self._context - cdef const char *kind = DPCTLUSM_GetPointerType( + cdef _usm_type UsmTy = DPCTLUSM_GetPointerType( self.memory_ptr, ctx.get_context_ref() ) - if kind == b'device': + if UsmTy == _usm_type._USM_DEVICE: raise ValueError("USM Device memory is not host accessible") buffer.buf = self.memory_ptr buffer.format = 'B' # byte @@ -547,11 +548,17 @@ cdef class _Memory: using the given context. Otherwise, returns b'shared', b'device', or b'host' type of the allocation. """ - cdef const char * usm_type = DPCTLUSM_GetPointerType( + cdef _usm_type usm_ty = DPCTLUSM_GetPointerType( p, ctx.get_context_ref() ) - - return usm_type + if usm_ty == _usm_type._USM_DEVICE: + return b'device' + elif usm_ty == _usm_type._USM_HOST: + return b'host' + elif usm_ty == _usm_type._USM_SHARED: + return b'shared' + else: + return b'unknown' @staticmethod cdef object create_from_usm_pointer_size_qref( @@ -569,7 +576,7 @@ cdef class _Memory: The object may not be a no-op dummy Python object to delay freeing the memory until later times. """ - cdef const char *usm_type + cdef _usm_type usm_ty cdef DPCTLSyclContextRef CRef = NULL cdef DPCTLSyclQueueRef QRef_copy = NULL cdef _Memory _mem @@ -581,13 +588,13 @@ cdef class _Memory: CRef = DPCTLQueue_GetContext(QRef) if (CRef is NULL): raise ValueError("Could not retrieve context from QRef") - usm_type = DPCTLUSM_GetPointerType(USMRef, CRef) + usm_ty = DPCTLUSM_GetPointerType(USMRef, CRef) DPCTLContext_Delete(CRef) - if usm_type == b"shared": + if usm_ty == _usm_type._USM_SHARED: mem_ty = MemoryUSMShared - elif usm_type == b"device": + elif usm_ty == _usm_type._USM_DEVICE: mem_ty = MemoryUSMDevice - elif usm_type == b"host": + elif usm_ty == _usm_type._USM_HOST: mem_ty = MemoryUSMHost else: raise ValueError( diff --git a/libsyclinterface/include/dpctl_sycl_enum_types.h b/libsyclinterface/include/dpctl_sycl_enum_types.h index fcf253d89b..87bb279d8a 100644 --- a/libsyclinterface/include/dpctl_sycl_enum_types.h +++ b/libsyclinterface/include/dpctl_sycl_enum_types.h @@ -31,6 +31,18 @@ DPCTL_C_EXTERN_C_BEGIN +/*! + * @brief Enum types for SYCL's USM allocator types. + * + */ +typedef enum +{ + DPCTL_USM_UNKNOWN = 0, + DPCTL_USM_DEVICE, + DPCTL_USM_HOST, + DPCTL_USM_SHARED +} DPCTLSyclUSMType; + /*! * @brief Redefinition of DPC++-specific Sycl backend types. * diff --git a/libsyclinterface/include/dpctl_sycl_usm_interface.h b/libsyclinterface/include/dpctl_sycl_usm_interface.h index 5a1b0ca483..04b00cbcb8 100644 --- a/libsyclinterface/include/dpctl_sycl_usm_interface.h +++ b/libsyclinterface/include/dpctl_sycl_usm_interface.h @@ -29,6 +29,7 @@ #include "Support/ExternC.h" #include "Support/MemOwnershipAttrs.h" #include "dpctl_data_types.h" +#include "dpctl_sycl_enum_types.h" #include "dpctl_sycl_types.h" DPCTL_C_EXTERN_C_BEGIN @@ -152,16 +153,17 @@ void DPCTLfree_with_context(__dpctl_take DPCTLSyclUSMRef MRef, __dpctl_keep const DPCTLSyclContextRef CRef); /*! - * @brief Get pointer type. + * @brief Returns the USM allocator type for a pointer. * - * @param MRef USM Memory + * @param MRef USM allocated pointer * @param CRef Sycl context reference associated with the pointer * - * @return "host", "device", "shared" or "unknown" + * @return DPCTLSyclUSMType enum value indicating if the pointer is of USM type + * "shared", "host", or "device". * @ingroup USMInterface */ DPCTL_API -const char * +DPCTLSyclUSMType DPCTLUSM_GetPointerType(__dpctl_keep const DPCTLSyclUSMRef MRef, __dpctl_keep const DPCTLSyclContextRef CRef); diff --git a/libsyclinterface/source/dpctl_sycl_usm_interface.cpp b/libsyclinterface/source/dpctl_sycl_usm_interface.cpp index 71561ab0bd..1b7a0c85ea 100644 --- a/libsyclinterface/source/dpctl_sycl_usm_interface.cpp +++ b/libsyclinterface/source/dpctl_sycl_usm_interface.cpp @@ -177,16 +177,17 @@ void DPCTLfree_with_context(__dpctl_take DPCTLSyclUSMRef MRef, free(Ptr, *C); } -const char *DPCTLUSM_GetPointerType(__dpctl_keep const DPCTLSyclUSMRef MRef, - __dpctl_keep const DPCTLSyclContextRef CRef) +DPCTLSyclUSMType +DPCTLUSM_GetPointerType(__dpctl_keep const DPCTLSyclUSMRef MRef, + __dpctl_keep const DPCTLSyclContextRef CRef) { if (!CRef) { error_handler("Input CRef is nullptr.", __FILE__, __func__, __LINE__); - return "unknown"; + return DPCTLSyclUSMType::DPCTL_USM_UNKNOWN; } if (!MRef) { error_handler("Input MRef is nullptr.", __FILE__, __func__, __LINE__); - return "unknown"; + return DPCTLSyclUSMType::DPCTL_USM_UNKNOWN; } auto Ptr = unwrap(MRef); auto C = unwrap(CRef); @@ -194,13 +195,13 @@ const char *DPCTLUSM_GetPointerType(__dpctl_keep const DPCTLSyclUSMRef MRef, auto kind = get_pointer_type(Ptr, *C); switch (kind) { case usm::alloc::host: - return "host"; + return DPCTLSyclUSMType::DPCTL_USM_HOST; case usm::alloc::device: - return "device"; + return DPCTLSyclUSMType::DPCTL_USM_DEVICE; case usm::alloc::shared: - return "shared"; + return DPCTLSyclUSMType::DPCTL_USM_SHARED; default: - return "unknown"; + return DPCTLSyclUSMType::DPCTL_USM_UNKNOWN; } } diff --git a/libsyclinterface/tests/test_sycl_usm_interface.cpp b/libsyclinterface/tests/test_sycl_usm_interface.cpp index 15b11f79e7..a6dbb2290a 100644 --- a/libsyclinterface/tests/test_sycl_usm_interface.cpp +++ b/libsyclinterface/tests/test_sycl_usm_interface.cpp @@ -45,12 +45,12 @@ constexpr size_t SIZE = 1024; void common_test_body(size_t nbytes, const DPCTLSyclUSMRef Ptr, const DPCTLSyclQueueRef Q, - const char *expected) + DPCTLSyclUSMType expected) { auto Ctx = DPCTLQueue_GetContext(Q); auto kind = DPCTLUSM_GetPointerType(Ptr, Ctx); - EXPECT_TRUE(0 == std::strncmp(kind, expected, 4)); + EXPECT_TRUE(kind == expected); auto Dev = DPCTLUSM_GetPointerDevice(Ptr, Ctx); auto QueueDev = DPCTLQueue_GetDevice(Q); @@ -100,7 +100,7 @@ TEST_F(TestDPCTLSyclUSMInterface, MallocShared) const size_t nbytes = SIZE; auto Ptr = DPCTLmalloc_shared(nbytes, Q); EXPECT_TRUE(bool(Ptr)); - common_test_body(nbytes, Ptr, Q, "shared"); + common_test_body(nbytes, Ptr, Q, DPCTLSyclUSMType::DPCTL_USM_SHARED); DPCTLfree_with_queue(Ptr, Q); DPCTLQueue_Delete(Q); } @@ -112,7 +112,7 @@ TEST_F(TestDPCTLSyclUSMInterface, MallocDevice) const size_t nbytes = SIZE; auto Ptr = DPCTLmalloc_device(nbytes, Q); EXPECT_TRUE(bool(Ptr)); - common_test_body(nbytes, Ptr, Q, "device"); + common_test_body(nbytes, Ptr, Q, DPCTLSyclUSMType::DPCTL_USM_DEVICE); DPCTLfree_with_queue(Ptr, Q); DPCTLQueue_Delete(Q); } @@ -124,7 +124,7 @@ TEST_F(TestDPCTLSyclUSMInterface, MallocHost) const size_t nbytes = SIZE; auto Ptr = DPCTLmalloc_host(nbytes, Q); EXPECT_TRUE(bool(Ptr)); - common_test_body(nbytes, Ptr, Q, "host"); + common_test_body(nbytes, Ptr, Q, DPCTLSyclUSMType::DPCTL_USM_HOST); DPCTLfree_with_queue(Ptr, Q); DPCTLQueue_Delete(Q); } @@ -136,7 +136,7 @@ TEST_F(TestDPCTLSyclUSMInterface, AlignedAllocShared) const size_t nbytes = SIZE; auto Ptr = DPCTLaligned_alloc_shared(64, nbytes, Q); EXPECT_TRUE(bool(Ptr)); - common_test_body(nbytes, Ptr, Q, "shared"); + common_test_body(nbytes, Ptr, Q, DPCTLSyclUSMType::DPCTL_USM_SHARED); DPCTLfree_with_queue(Ptr, Q); DPCTLQueue_Delete(Q); } @@ -148,7 +148,7 @@ TEST_F(TestDPCTLSyclUSMInterface, AlignedAllocDevice) const size_t nbytes = SIZE; auto Ptr = DPCTLaligned_alloc_device(64, nbytes, Q); EXPECT_TRUE(bool(Ptr)); - common_test_body(nbytes, Ptr, Q, "device"); + common_test_body(nbytes, Ptr, Q, DPCTLSyclUSMType::DPCTL_USM_DEVICE); DPCTLfree_with_queue(Ptr, Q); DPCTLQueue_Delete(Q); } @@ -160,7 +160,7 @@ TEST_F(TestDPCTLSyclUSMInterface, AlignedAllocHost) const size_t nbytes = SIZE; auto Ptr = DPCTLaligned_alloc_host(64, nbytes, Q); EXPECT_TRUE(bool(Ptr)); - common_test_body(nbytes, Ptr, Q, "host"); + common_test_body(nbytes, Ptr, Q, DPCTLSyclUSMType::DPCTL_USM_HOST); DPCTLfree_with_queue(Ptr, Q); } @@ -234,13 +234,10 @@ TEST_F(TestDPCTLSyclUSMNullArgs, ChkPointerQueries) { DPCTLSyclContextRef Null_CRef = nullptr; DPCTLSyclUSMRef Null_MRef = nullptr; - const char *t = nullptr; - auto is_unknown = [=](const char *t) -> bool { - return strncmp(t, "unknown", 7) == 0; - }; + DPCTLSyclUSMType t = DPCTLSyclUSMType::DPCTL_USM_UNKNOWN; EXPECT_NO_FATAL_FAILURE(t = DPCTLUSM_GetPointerType(Null_MRef, Null_CRef)); - ASSERT_TRUE(is_unknown(t)); + ASSERT_TRUE(t == DPCTLSyclUSMType::DPCTL_USM_UNKNOWN); DPCTLSyclDeviceSelectorRef DSRef = nullptr; DPCTLSyclDeviceRef DRef = nullptr; @@ -253,9 +250,8 @@ TEST_F(TestDPCTLSyclUSMNullArgs, ChkPointerQueries) EXPECT_NO_FATAL_FAILURE(CRef = DPCTLContext_Create(DRef, nullptr, 0)); EXPECT_NO_FATAL_FAILURE(DPCTLDevice_Delete(DRef)); - t = nullptr; EXPECT_NO_FATAL_FAILURE(t = DPCTLUSM_GetPointerType(Null_MRef, CRef)); - ASSERT_TRUE(is_unknown(t)); + ASSERT_TRUE(t == DPCTLSyclUSMType::DPCTL_USM_UNKNOWN); DPCTLSyclDeviceRef D2Ref = nullptr; EXPECT_NO_FATAL_FAILURE(D2Ref = DPCTLUSM_GetPointerDevice(Null_MRef, CRef));