Skip to content
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.

Sign up for GitHub

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

[UR][Bindless] Initial implementation of bindless images for HIP #2496

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[UR][Bindless] Initial implementation of bindless images for HIP #2496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions source/adapters/hip/common.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ ur_result_t mapErrorUR(hipError_t Result) {
return UR_RESULT_ERROR_OUT_OF_HOST_MEMORY;
case hipErrorLaunchOutOfResources:
return UR_RESULT_ERROR_OUT_OF_RESOURCES;
case hipErrorNotInitialized:
return UR_RESULT_ERROR_UNINITIALIZED;
case hipErrorNotSupported:
return UR_RESULT_ERROR_UNSUPPORTED_FEATURE;
default:
return UR_RESULT_ERROR_UNKNOWN;
}
Expand Down
140 changes: 140 additions & 0 deletions source/adapters/hip/device.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "event.hpp"
#include "logger/ur_logger.hpp"

#include <hip/hip_runtime.h>
#include <sstream>

int getAttribute(ur_device_handle_t Device, hipDeviceAttribute_t Attribute) {
Expand Down Expand Up @@ -785,6 +786,145 @@ UR_APIEXPORT ur_result_t UR_APICALL urDeviceGetInfo(ur_device_handle_t hDevice,
return ReturnValue(int32_t{1});
}

case UR_DEVICE_INFO_BINDLESS_IMAGES_SUPPORT_EXP: {
// On HIP bindless images are implemented but support is device-dependent.
return ReturnValue(
static_cast<ur_bool_t>(hDevice->supportsHardwareImages()));
}
case UR_DEVICE_INFO_BINDLESS_IMAGES_SHARED_USM_SUPPORT_EXP: {
// On HIP bindless images can be backed by shared (managed) USM.
return ReturnValue(
static_cast<ur_bool_t>(hDevice->supportsHardwareImages()));
}
case UR_DEVICE_INFO_BINDLESS_IMAGES_1D_USM_SUPPORT_EXP: {
// On HIP 1D bindless image USM is supported, but sampling is not.
// More specifically, image creation from with sampler using linear
// filtering is unstable and somtimes passes while other times returns
// unsupported error code.
return ReturnValue(
static_cast<ur_bool_t>(hDevice->supportsHardwareImages()));
}
case UR_DEVICE_INFO_BINDLESS_IMAGES_2D_USM_SUPPORT_EXP: {
// On HIP 2D bindless image USM is supported, but sampling is not.
// More specifically, image creation from with sampler using linear
// filtering is unstable and somtimes passes while other times returns
// unsupported error code.
return ReturnValue(
static_cast<ur_bool_t>(hDevice->supportsHardwareImages()));
}
case UR_DEVICE_INFO_IMAGE_PITCH_ALIGN_EXP: {
int32_t tex_pitch_align{0};
UR_CHECK_ERROR(hipDeviceGetAttribute(
&tex_pitch_align, hipDeviceAttributeTexturePitchAlignment,
hDevice->get()));
detail::ur::assertion(tex_pitch_align >= 0);
return ReturnValue(static_cast<uint32_t>(tex_pitch_align));
}
case UR_DEVICE_INFO_MAX_IMAGE_LINEAR_WIDTH_EXP: {
// Default values due to non-existent hipamd queries for linear sizes.
constexpr uint32_t MaxLinearWidth{1};
return ReturnValue(MaxLinearWidth);
}
case UR_DEVICE_INFO_MAX_IMAGE_LINEAR_HEIGHT_EXP: {
// Default values due to non-existent hipamd queries for linear sizes.
constexpr uint32_t MaxLinearHeight{1};
return ReturnValue(MaxLinearHeight);
}
case UR_DEVICE_INFO_MAX_IMAGE_LINEAR_PITCH_EXP: {
// Default values due to non-existent hipamd queries for linear sizes.
constexpr uint32_t MaxLinearPitch{1};
return ReturnValue(MaxLinearPitch);
}
case UR_DEVICE_INFO_MIPMAP_SUPPORT_EXP: {
// HIP supports mipmaps.
// TODO: DPC++ doesn't implement the required mipmap builtins for SYCL.
return ReturnValue(ur_bool_t{false});
}
case UR_DEVICE_INFO_MIPMAP_ANISOTROPY_SUPPORT_EXP: {
// HIP supports anisotropic filtering.
// TODO: DPC++ doesn't implement the required mipmap builtins for SYCL.
return ReturnValue(ur_bool_t{false});
}
case UR_DEVICE_INFO_MIPMAP_MAX_ANISOTROPY_EXP: {
// HIP has no query for this, but documentation states max value is 16.
constexpr float MaxAnisotropy{16.f};
return ReturnValue(MaxAnisotropy);
}
case UR_DEVICE_INFO_MIPMAP_LEVEL_REFERENCE_SUPPORT_EXP: {
// HIP supports creation of images from individual mipmap levels.
// TODO: DPC++ doesn't implement the required mipmap builtins for SYCL.
return ReturnValue(ur_bool_t{false});
}

case UR_DEVICE_INFO_EXTERNAL_MEMORY_IMPORT_SUPPORT_EXP: {
// Importing external memory is supported, but there are current
// issues in the HIP runtime due to which mapping the external array memory
// does not work at the moment. Linear/buffer memory mapping is supported.
return ReturnValue(ur_bool_t{false});
}
case UR_DEVICE_INFO_EXTERNAL_SEMAPHORE_IMPORT_SUPPORT_EXP: {
// HIP supports importing external semaphores.
// TODO: Importing external semaphores should be supported in the adapter,
// but there are still current issues as not all related tests are passing.
return ReturnValue(ur_bool_t{false});
}
case UR_DEVICE_INFO_CUBEMAP_SUPPORT_EXP: {
// HIP supports cubemaps.
// TODO: DPC++ doesn't implement the required builtins for SYCL.
return ReturnValue(ur_bool_t{false});
}
case UR_DEVICE_INFO_CUBEMAP_SEAMLESS_FILTERING_SUPPORT_EXP: {
// HIP supports cubemap seamless filtering.
// TODO: DPC++ doesn't implement the required builtins for SYCL.
return ReturnValue(ur_bool_t{false});
}
case UR_DEVICE_INFO_BINDLESS_SAMPLED_IMAGE_FETCH_1D_USM_EXP: {
// HIP does support fetching 1D USM sampled image data.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a personal preference but I prefer supports (as in the comment(s) above) to does support (below). I initially parsed it as does not support.

// TODO: DPC++ doesn't implement the required builtins for SYCL.
return ReturnValue(ur_bool_t{false});
}
case UR_DEVICE_INFO_BINDLESS_SAMPLED_IMAGE_FETCH_1D_EXP: {
// HIP does not support fetching 1D non-USM sampled image data.
// TODO: DPC++ doesn't implement the required builtins for SYCL.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to have a TODO here if HIP doesn't support it anyway?

return ReturnValue(ur_bool_t{false});
}
case UR_DEVICE_INFO_BINDLESS_SAMPLED_IMAGE_FETCH_2D_USM_EXP: {
// HIP does support fetching 2D USM sampled image data.
// TODO: DPC++ doesn't implement the required builtins for SYCL.
return ReturnValue(ur_bool_t{false});
}
case UR_DEVICE_INFO_BINDLESS_SAMPLED_IMAGE_FETCH_2D_EXP: {
// HIP does support fetching 2D non-USM sampled image data.
// TODO: DPC++ doesn't implement the required builtins for SYCL.
return ReturnValue(ur_bool_t{false});
}
case UR_DEVICE_INFO_BINDLESS_SAMPLED_IMAGE_FETCH_3D_EXP: {
// HIP does support fetching 3D non-USM sampled image data.
// TODO: DPC++ doesn't implement the required builtins for SYCL.
return ReturnValue(ur_bool_t{false});
}
case UR_DEVICE_INFO_IMAGE_ARRAY_SUPPORT_EXP: {
// TODO: Our HIP adapter implements image arrays but DPC++ end-to-end
// testing currently fails due to various runtime errors that could be
// arch-specific driver support, as well missing builtins. Hence, this
// feature is marked unsupported until those issues are resolved.
return ReturnValue(ur_bool_t{false});
}
case UR_DEVICE_INFO_BINDLESS_UNIQUE_ADDRESSING_PER_DIM_EXP: {
// HIP does not support unique addressing per dimension
return ReturnValue(ur_bool_t{false});
}
case UR_DEVICE_INFO_BINDLESS_SAMPLE_1D_USM_EXP: {
// HIP does support sampling 1D USM sampled image data.
return ReturnValue(
static_cast<ur_bool_t>(hDevice->supportsHardwareImages()));
}
case UR_DEVICE_INFO_BINDLESS_SAMPLE_2D_USM_EXP: {
// HIP does support sampling 2D USM sampled image data.
return ReturnValue(
static_cast<ur_bool_t>(hDevice->supportsHardwareImages()));
}

case UR_DEVICE_INFO_KERNEL_SET_SPECIALIZATION_CONSTANTS: {
return ReturnValue(ur_bool_t{false});
}
Expand Down
14 changes: 14 additions & 0 deletions source/adapters/hip/device.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

#include <ur/ur.hpp>

#include <map>

/// UR device mapping to a hipDevice_t.
/// Includes an observer pointer to the platform,
/// and implements the reference counting semantics since
Expand All @@ -34,6 +36,7 @@ struct ur_device_handle_t_ {
int DeviceMaxLocalMem{0};
int ManagedMemSupport{0};
int ConcurrentManagedAccess{0};
int HardwareImageSupport{0};

public:
ur_device_handle_t_(native_type HipDevice, hipEvent_t EvBase,
Expand All @@ -57,6 +60,10 @@ struct ur_device_handle_t_ {
UR_CHECK_ERROR(hipDeviceGetAttribute(
&ConcurrentManagedAccess, hipDeviceAttributeConcurrentManagedAccess,
HIPDevice));
// Check if texture functions are supported in the HIP host runtime.
UR_CHECK_ERROR(hipDeviceGetAttribute(
&HardwareImageSupport, hipDeviceAttributeImageSupport, HIPDevice));
detail::ur::assertion(HardwareImageSupport >= 0);
}

~ur_device_handle_t_() noexcept(false) {}
Expand Down Expand Up @@ -88,6 +95,13 @@ struct ur_device_handle_t_ {
int getConcurrentManagedAccess() const noexcept {
return ConcurrentManagedAccess;
};

bool supportsHardwareImages() const noexcept {
return HardwareImageSupport ? true : false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a little clunky. I see that hipDeviceAttributeImageSupport returns ‘1’ if Device supports image, ‘0’ otherwise. (source).

Perhaps we could store HardwareImageSupport as a boolean, and format the return value of hipDeviceGetAttribute once?

    // Check if texture functions are supported in the HIP host runtime.
    int ret;
    UR_CHECK_ERROR(hipDeviceGetAttribute(
        &ret, hipDeviceAttributeImageSupport, HIPDevice));
    detail::ur::assertion(ret == 0 || ret == 1);
    /*(bool)*/ HardwareImageSupport = ret == 1;

? Or just return HardwareImageSupport == 1 here I guess.

Copy link
Contributor Author

@GeorgeWeb GeorgeWeb Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there's another one (attribute member) stored as integer but used as boolean too here. Anyways, since this one is going to be used as boolean in any situation I can imagine, it may make more sense to just store as boolean. I'll do that. Thanks.

}

// Used for bookkeeping for mipmapped array leaks in mapping external memory.
std::map<hipArray_t, hipMipmappedArray_t> ChildHipArrayFromMipmapMap;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the order important? Would a std::unordered_map bring any benefits (it's usually faster)?

Copy link
Contributor Author

@GeorgeWeb GeorgeWeb Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair question. I don't know the answer to this, as it is a change I was asked to adapt from the Cuda backend implementation that fixes a race/leak. I will double check and come back to it. Hence, I didn't change the data structure. I will double check with the team to see if the order is important and if not I'll switch to std::unordered_map.

};

int getAttribute(ur_device_handle_t Device, hipDeviceAttribute_t Attribute);
Expand Down
Loading
Loading