[SYCL][Bindless] Deprecate standalone mipmap alloc and free (#11149)

This MR aligns the implementation with the proposal by deprecating the
standalone mipmap alloc and free functions.

It also slightly modifies the wording in the proposal to state that the
correct image type is allocated using `alloc_image_mem` and the
`image_type`.

There is also a minor typo fix in both the impl and proposal.

Author: Seanst98

sycl/doc/extensions/experimental/sycl_ext_oneapi_bindless_images.asciidoc

@@ -322,8 +322,8 @@ void free_image_mem(image_mem_handle memHandle,
 
 The first method of allocating device memory for images is through 
 `alloc_image_mem`. This takes a `sycl::device`, `sycl::context`, 
-and `image_descriptor` to allocate device memory, appropriately sized 
-based on the `image_descriptor`. Alternatively, we can also pass a 
+and `image_descriptor` to allocate device memory, with the appropriate image 
+type and size based on the `image_descriptor`. Alternatively, we can also pass a 
 `sycl::queue` instead of both `sycl::device` and `sycl::context`.
 
 Memory allocated in this way requires the user to free that memory after all 
@@ -673,7 +673,7 @@ sample, with the minimum value being 0.
 `max_mipmap_level_clamp` defines the maximum mipmap level from which we can 
 sample. This cannot value cannot be higher than the number of allocated levels.
 
-`max_anisotropy` dictates the anisotropic ratio used when samplling the mipmap 
+`max_anisotropy` dictates the anisotropic ratio used when sampling the mipmap 
 with anisotropic filtering.
 
 === Explicit copies
@@ -1975,4 +1975,5 @@ These features still need to be handled:
                  - Remove support for packed normalized image formats 
                    (`unorm_short_555`, `unorm_short_565`, `unorm_int_101010`)
 |4.4|2023-09-12| - Added overload with `sycl::queue` to standalone functions
+|4.5|2023-09-14| - Update wording for allocating images + fix typo
 |======================

sycl/include/sycl/ext/oneapi/bindless_images.hpp

@@ -61,67 +61,99 @@ alloc_image_mem(const image_descriptor &desc, const sycl::device &syclDevice,
 __SYCL_EXPORT image_mem_handle alloc_image_mem(const image_descriptor &desc,
                                                const sycl::queue &syclQueue);
 
+/**
+ *  @brief   [Deprecated] Free image memory
+ *
+ *  @param   handle Memory handle to allocated memory on the device
+ *  @param   syclDevice The device in which we create our memory handle
+ *  @param   syclContext The context in which we created our memory handle
+ */
+__SYCL_EXPORT_DEPRECATED("Distinct image frees are deprecated. "
+                         "Instead use overload that accepts image_type.")
+void free_image_mem(image_mem_handle handle, const sycl::device &syclDevice,
+                    const sycl::context &syclContext);
+
+/**
+ *  @brief   [Deprecated] Free image memory
+ *
+ *  @param   handle Memory handle to allocated memory on the device
+ *  @param   syclQueue The queue in which we create our memory handle
+ */
+__SYCL_EXPORT_DEPRECATED("Distinct image frees are deprecated. "
+                         "Instead use overload that accepts image_type.")
+void free_image_mem(image_mem_handle handle, const sycl::device &syclQueue);
+
 /**
  *  @brief   Free image memory
  *
  *  @param   handle Memory handle to allocated memory on the device
+ *  @param   imageType Type of image memory to be freed
  *  @param   syclDevice The device in which we create our memory handle
  *  @param   syclContext The context in which we created our memory handle
  */
-__SYCL_EXPORT void free_image_mem(image_mem_handle handle,
+__SYCL_EXPORT void free_image_mem(image_mem_handle handle, image_type imageType,
                                   const sycl::device &syclDevice,
                                   const sycl::context &syclContext);
 
 /**
  *  @brief   Free image memory
  *
  *  @param   handle Memory handle to allocated memory on the device
+ *  @param   imageType Type of image memory to be freed
  *  @param   syclQueue The queue in which we create our memory handle
  */
-__SYCL_EXPORT void free_image_mem(image_mem_handle handle,
-                                  const sycl::device &syclQueue);
+__SYCL_EXPORT void free_image_mem(image_mem_handle handle, image_type imageType,
+                                  const sycl::queue &syclQueue);
 
 /**
- *  @brief   Allocate mipmap memory based on image_descriptor
+ *  @brief   [Deprecated] Allocate mipmap memory based on image_descriptor
  *
  *  @param   desc The image descriptor
  *  @param   syclDevice The device in which we create our memory handle
  *  @param   syclContext The context in which we create our memory handle
  *  @return  Memory handle to allocated memory on the device
  */
-__SYCL_EXPORT image_mem_handle
-alloc_mipmap_mem(const image_descriptor &desc, const sycl::device &syclDevice,
-                 const sycl::context &syclContext);
+__SYCL_EXPORT_DEPRECATED("Distinct mipmap allocs are deprecated. "
+                         "Instead use alloc_image_mem().")
+image_mem_handle alloc_mipmap_mem(const image_descriptor &desc,
+                                  const sycl::device &syclDevice,
+                                  const sycl::context &syclContext);
 
 /**
- *  @brief   Allocate mipmap memory based on image_descriptor
+ *  @brief   [Deprecated] Allocate mipmap memory based on image_descriptor
  *
  *  @param   desc The image descriptor
  *  @param   syclQueue The queue in which we create our memory handle
  *  @return  Memory handle to allocated memory on the device
  */
-__SYCL_EXPORT image_mem_handle alloc_mipmap_mem(const image_descriptor &desc,
-                                                const sycl::device &syclQueue);
+__SYCL_EXPORT_DEPRECATED("Distinct mipmap allocs are deprecated. "
+                         "Instead use alloc_image_mem().")
+image_mem_handle alloc_mipmap_mem(const image_descriptor &desc,
+                                  const sycl::device &syclQueue);
 
 /**
- *  @brief   Free mipmap memory
+ *  @brief   [Deprecated] Free mipmap memory
  *
  *  @param   handle The mipmap memory handle
  *  @param   syclDevice The device in which we created our memory handle
  *  @param   syclContext The context in which we created our memory handle
  */
-__SYCL_EXPORT void free_mipmap_mem(image_mem_handle handle,
-                                   const sycl::device &syclDevice,
-                                   const sycl::context &syclContext);
+__SYCL_EXPORT_DEPRECATED(
+    "Distinct mipmap frees are deprecated. "
+    "Instead use free_image_mem() that accepts image_type.")
+void free_mipmap_mem(image_mem_handle handle, const sycl::device &syclDevice,
+                     const sycl::context &syclContext);
 
 /**
- *  @brief   Free mipmap memory
+ *  @brief   [Deprecated] Free mipmap memory
  *
  *  @param   handle The mipmap memory handle
  *  @param   syclQueue The queue in which we created our memory handle
  */
-__SYCL_EXPORT void free_mipmap_mem(image_mem_handle handle,
-                                   const sycl::queue &syclQueue);
+__SYCL_EXPORT_DEPRECATED(
+    "Distinct mipmap frees are deprecated. "
+    "Instead use free_image_mem() that accepts image_type.")
+void free_mipmap_mem(image_mem_handle handle, const sycl::queue &syclQueue);
 
 /**
  *  @brief   Retrieve the memory handle to an individual mipmap image
@@ -205,7 +237,7 @@ __SYCL_EXPORT image_mem_handle map_external_memory_array(
  *  @return  Memory handle to externally allocated memory on the device
  */
 __SYCL_EXPORT image_mem_handle map_external_memory_array(
-    interop_mem_handle memHandle, const image_descriptor &descm,
+    interop_mem_handle memHandle, const image_descriptor &desc,
     const sycl::queue &syclQueue);
 
 /**

sycl/source/detail/bindless_images.cpp

@@ -49,21 +49,12 @@ detail::image_mem_impl::image_mem_impl(const image_descriptor &desc,
                                        const device &syclDevice,
                                        const context &syclContext)
     : descriptor(desc), syclDevice(syclDevice), syclContext(syclContext) {
-  if (desc.type == image_type::mipmap) {
-    handle = alloc_mipmap_mem(desc, syclDevice, syclContext);
-  } else {
-    handle = alloc_image_mem(desc, syclDevice, syclContext);
-  }
+  handle = alloc_image_mem(desc, syclDevice, syclContext);
 }
 
 detail::image_mem_impl::~image_mem_impl() {
-  if (handle.raw_handle != nullptr) {
-    if (descriptor.type == image_type::mipmap) {
-      free_mipmap_mem(handle, syclDevice, syclContext);
-    } else {
-      free_image_mem(handle, syclDevice, syclContext);
-    }
-  }
+  free_image_mem(this->get_handle(), this->get_descriptor().type,
+                 this->get_device(), this->get_context());
 }
 
 __SYCL_EXPORT
@@ -163,10 +154,21 @@ alloc_image_mem(const image_descriptor &desc, const sycl::device &syclDevice,
   pi_device Device = DevImpl->getHandleRef();
   const sycl::detail::PluginPtr &Plugin = CtxImpl->getPlugin();
 
-  // Non-mipmap images must have only 1 level
-  if (desc.num_levels != 1)
+  if (desc.type == image_type::mipmap) {
+    // Mipmaps must have more than one level
+    if (desc.num_levels <= 1)
+      throw sycl::exception(sycl::make_error_code(sycl::errc::invalid),
+                            "Mipmap number of levels must be 2 or more");
+  } else if (desc.type == image_type::standard) {
+    // Non-mipmap images must have only 1 level
+    if (desc.num_levels != 1)
+      throw sycl::exception(sycl::make_error_code(sycl::errc::invalid),
+                            "Image number of levels must be 1");
+  } else {
+    // Not an image to allocate
     throw sycl::exception(sycl::make_error_code(sycl::errc::invalid),
-                          "Image number of levels must be 1");
+                          "Invalid image type to allocate");
+  }
 
   pi_image_desc piDesc;
   pi_image_format piFormat;
@@ -187,9 +189,11 @@ __SYCL_EXPORT image_mem_handle alloc_image_mem(const image_descriptor &desc,
   return alloc_image_mem(desc, syclQueue.get_device(), syclQueue.get_context());
 }
 
-__SYCL_EXPORT image_mem_handle
-alloc_mipmap_mem(const image_descriptor &desc, const sycl::device &syclDevice,
-                 const sycl::context &syclContext) {
+__SYCL_EXPORT_DEPRECATED("Distinct mipmap allocs are deprecated. "
+                         "Instead use alloc_image_mem().")
+image_mem_handle alloc_mipmap_mem(const image_descriptor &desc,
+                                  const sycl::device &syclDevice,
+                                  const sycl::context &syclContext) {
   std::shared_ptr<sycl::detail::context_impl> CtxImpl =
       sycl::detail::getSyclObjImpl(syclContext);
   pi_context C = CtxImpl->getHandleRef();
@@ -216,8 +220,10 @@ alloc_mipmap_mem(const image_descriptor &desc, const sycl::device &syclDevice,
   return retHandle;
 }
 
-__SYCL_EXPORT image_mem_handle alloc_mipmap_mem(const image_descriptor &desc,
-                                                const sycl::queue &syclQueue) {
+__SYCL_EXPORT_DEPRECATED("Distinct mipmap allocs are deprecated. "
+                         "Instead use alloc_image_mem().")
+image_mem_handle alloc_mipmap_mem(const image_descriptor &desc,
+                                  const sycl::queue &syclQueue) {
   return alloc_mipmap_mem(desc, syclQueue.get_device(),
                           syclQueue.get_context());
 }
@@ -251,6 +257,7 @@ get_mip_level_mem_handle(const image_mem_handle mipMem, unsigned int level,
 }
 
 __SYCL_EXPORT void free_image_mem(image_mem_handle memHandle,
+                                  image_type imageType,
                                   const sycl::device &syclDevice,
                                   const sycl::context &syclContext) {
   std::shared_ptr<sycl::detail::context_impl> CtxImpl =
@@ -261,19 +268,49 @@ __SYCL_EXPORT void free_image_mem(image_mem_handle memHandle,
   pi_device Device = DevImpl->getHandleRef();
   const sycl::detail::PluginPtr &Plugin = CtxImpl->getPlugin();
 
-  Plugin->call<sycl::errc::memory_allocation,
-               sycl::detail::PiApiKind::piextMemImageFree>(
-      C, Device, memHandle.raw_handle);
+  if (memHandle.raw_handle != nullptr) {
+    if (imageType == image_type::mipmap) {
+      Plugin->call<sycl::errc::memory_allocation,
+                   sycl::detail::PiApiKind::piextMemMipmapFree>(
+          C, Device, memHandle.raw_handle);
+    } else if (imageType == image_type::standard) {
+      Plugin->call<sycl::errc::memory_allocation,
+                   sycl::detail::PiApiKind::piextMemImageFree>(
+          C, Device, memHandle.raw_handle);
+    } else {
+      throw sycl::exception(sycl::make_error_code(sycl::errc::invalid),
+                            "Invalid image type to free");
+    }
+  }
 }
 
 __SYCL_EXPORT void free_image_mem(image_mem_handle memHandle,
+                                  image_type imageType,
                                   const sycl::queue &syclQueue) {
+  free_image_mem(memHandle, imageType, syclQueue.get_device(),
+                 syclQueue.get_context());
+}
+
+__SYCL_EXPORT_DEPRECATED("Distinct image frees are deprecated. "
+                         "Instead use overload that accepts image_type.")
+void free_image_mem(image_mem_handle memHandle, const sycl::device &syclDevice,
+                    const sycl::context &syclContext) {
+  return free_image_mem(memHandle, image_type::standard, syclDevice,
+                        syclContext);
+}
+
+__SYCL_EXPORT_DEPRECATED("Distinct image frees are deprecated. "
+                         "Instead use overload that accepts image_type.")
+void free_image_mem(image_mem_handle memHandle, const sycl::queue &syclQueue) {
   free_image_mem(memHandle, syclQueue.get_device(), syclQueue.get_context());
 }
 
-__SYCL_EXPORT void free_mipmap_mem(image_mem_handle memoryHandle,
-                                   const sycl::device &syclDevice,
-                                   const sycl::context &syclContext) {
+__SYCL_EXPORT_DEPRECATED(
+    "Distinct mipmap frees are deprecated. "
+    "Instead use free_image_mem() that accepts image_type.")
+void free_mipmap_mem(image_mem_handle memoryHandle,
+                     const sycl::device &syclDevice,
+                     const sycl::context &syclContext) {
   std::shared_ptr<sycl::detail::context_impl> CtxImpl =
       sycl::detail::getSyclObjImpl(syclContext);
   pi_context C = CtxImpl->getHandleRef();
@@ -287,8 +324,11 @@ __SYCL_EXPORT void free_mipmap_mem(image_mem_handle memoryHandle,
       C, Device, memoryHandle.raw_handle);
 }
 
-__SYCL_EXPORT void free_mipmap_mem(image_mem_handle memoryHandle,
-                                   const sycl::queue &syclQueue) {
+__SYCL_EXPORT_DEPRECATED(
+    "Distinct mipmap frees are deprecated. "
+    "Instead use free_image_mem() that accepts image_type.")
+void free_mipmap_mem(image_mem_handle memoryHandle,
+                     const sycl::queue &syclQueue) {
   free_mipmap_mem(memoryHandle, syclQueue.get_device(),
                   syclQueue.get_context());
 }

sycl/test/abi/sycl_symbols_linux.dump

@@ -3671,6 +3671,8 @@ _ZN4sycl3_V13ext6oneapi12experimental12create_imageERNS3_9image_memERKNS3_16imag
 _ZN4sycl3_V13ext6oneapi12experimental12create_imageERNS3_9image_memERKNS3_16image_descriptorERKNS0_6deviceERKNS0_7contextE
 _ZN4sycl3_V13ext6oneapi12experimental12create_imageERNS3_9image_memERKNS3_22bindless_image_samplerERKNS3_16image_descriptorERKNS0_5queueE
 _ZN4sycl3_V13ext6oneapi12experimental12create_imageERNS3_9image_memERKNS3_22bindless_image_samplerERKNS3_16image_descriptorERKNS0_6deviceERKNS0_7contextE
+_ZN4sycl3_V13ext6oneapi12experimental14free_image_memENS3_16image_mem_handleENS3_10image_typeERKNS0_5queueE
+_ZN4sycl3_V13ext6oneapi12experimental14free_image_memENS3_16image_mem_handleENS3_10image_typeERKNS0_6deviceERKNS0_7contextE
 _ZN4sycl3_V13ext6oneapi12experimental14free_image_memENS3_16image_mem_handleERKNS0_5queueE
 _ZN4sycl3_V13ext6oneapi12experimental14free_image_memENS3_16image_mem_handleERKNS0_6deviceERKNS0_7contextE
 _ZN4sycl3_V13ext6oneapi12experimental15alloc_image_memERKNS3_16image_descriptorERKNS0_5queueE

sycl/test/abi/sycl_symbols_windows.dump

@@ -1087,6 +1087,8 @@
 ?free@_V1@sycl@@YAXPEAXAEBVqueue@12@AEBUcode_location@detail@12@@Z
 ?free_image_mem@experimental@oneapi@ext@_V1@sycl@@YAXUimage_mem_handle@12345@AEBVdevice@45@AEBVcontext@45@@Z
 ?free_image_mem@experimental@oneapi@ext@_V1@sycl@@YAXUimage_mem_handle@12345@AEBVqueue@45@@Z
+?free_image_mem@experimental@oneapi@ext@_V1@sycl@@YAXUimage_mem_handle@12345@W4image_type@12345@AEBVdevice@45@AEBVcontext@45@@Z
+?free_image_mem@experimental@oneapi@ext@_V1@sycl@@YAXUimage_mem_handle@12345@W4image_type@12345@AEBVqueue@45@@Z
 ?free_mipmap_mem@experimental@oneapi@ext@_V1@sycl@@YAXUimage_mem_handle@12345@AEBVdevice@45@AEBVcontext@45@@Z
 ?free_mipmap_mem@experimental@oneapi@ext@_V1@sycl@@YAXUimage_mem_handle@12345@AEBVqueue@45@@Z
 ?get@context@_V1@sycl@@QEBAPEAU_cl_context@@XZ