[SYCL][Ext][Bindless] Initial implementation of image spirv builtins on HIP #16439

GeorgeWeb · 2024-12-20T13:37:31Z

This PR implements the required spirv builtins in the libclc for AMD targets to support fetching (excluding sampled fetch for now), sampling, and writing for images and image arrays.

Additionally, End-to-end tests are updated to require the aspects corresponding to the feature that is being tested from the Bindless Images extension. This helps avoid having to manually say which backend adapter is supported or unsupported and instead rely on support based on aspect/device queries to drive the execution of the tests.

HIP adapter implementation in UR: oneapi-src/unified-runtime#2496

Signed-off-by: Georgi Mirazchiyski [email protected]

GeorgeWeb · 2025-01-07T12:19:14Z

I appreciate that image.cl is quite large and can only grow more (e.g. mipmaps, cubemaps, etc.) in the future, so I can consider splitting into image.cl for image/sampling functions, image_array.cl for image array/array sampling functions and image_common.h to hold all the commonly shared macros and functions.

jchlanda

LGTM.

This patch brings in a lot of new hand mangled names, was wondering if it would be at all possible to try and avoid those?

jchlanda · 2025-01-07T11:55:55Z

libclc/libspirv/lib/amdgcn-amdhsa/images/image.cl

+#endif
+
+#ifdef _WIN32
+#define _CLC_MANGLE_FUNC_IMG_HANDLE(namelength, name, prefix, postfix)              \


Isn't that the kind of thing that the remangler should handle, as in m - y, long (long) unsigned?

@jchlanda I agree that it should probably be the case.
We have that tendency of manually mangling these, for example, also in the atomics libspirv function implementations. This particular case here is adapted from the libspirv ptx image implementation. I think it is the perfect candidate to create a follow-up for, where we attempt to cleanup some of the manual mangling (at least for the image functions) across both ptx and amdgcn libspirv target implementations. I'll create an internal ticket for our team to follow this up. Does that sound okay?

I guess it's fine. My point was that if the only reason for manually mangled string is the m - y difference, that should work out of the box now. I'm slightly worried that if you found yourself in the situation that it indeed is needed, that means that somehow that case has escaped remangler's attention, and so the remangler needs more work.

Oh I think I see what you meant.
Well I wasn't too sure on that because I was getting Unable to demangle name: errors without m/y.
Mind if I ask for a quick code change suggestion in case I've tested it somehow wrongly?
Thanks!

I don't think you can get it to only skip the unsigned long/unsigned long long, the whole macro would need to be removed. So maybe it's best to do it as you suggest in a follow up.

If you have an easy way of reproducing that remangling error though, I'd be interested in that file (it should be possible to isolate it to a single function definition in IR).

jchlanda · 2025-01-08T08:05:34Z

I appreciate that image.cl is quite large and can only grow more (e.g. mipmaps, cubemaps, etc.) in the future, so I can consider splitting into image.cl for image/sampling functions, image_array.cl for image array/array sampling functions and image_common.h to hold all the commonly shared macros and functions.

I think that would be a good idea.

sycl/test-e2e/bindless_images/array/read_write_1d_subregion.cpp

sycl/test-e2e/bindless_images/read_sampled.cpp

This PR implements the entry points in the HIP adapter for the Bindless Images extension. Some features, while implemented, only partially pass the End-to-end tests for them in DPC++ (the PR that enables the support and testing in DPC++: intel/llvm#16439). This is because the HIP runtime doesn't support, for example, some parameter combinations with some image memory types. The not-fully supported features are marked with TODOs in the device info queries for further work. Signed-off-by: Georgi Mirazchiyski <[email protected]> Co-authored-by: Peter Žužek <[email protected]>

…on HIP This PR implements the required spirv builtins in the libclc for AMD targets to support image fetching (excluding sampled fetch for now) and sampling as well as image array fetching and sampling. Additionally, End-to-end tests are updated to require the aspects corresponding to the feature that is being tested from the Bindless Images extension. This helps avoid having to manually say which backend adapter is supported or unsupported and instead rely on support based on aspect/device queries to drive the execution of the tests. Signed-off-by: Georgi Mirazchiyski <[email protected]>

Signed-off-by: Georgi Mirazchiyski <[email protected]>

JackAKirk · 2025-02-05T17:01:21Z

marking as draft due to cubemap aspect failure.

Signed-off-by: JackAKirk <[email protected]>

JackAKirk · 2025-02-05T17:38:51Z

Apologies @cperkinsintel for the request. It was a mistake this doesn't require your review.

Signed-off-by: JackAKirk <[email protected]>

ldrumm · 2025-02-06T14:40:30Z

I'd like to get @frasercrmck's opinion on the libclc changes before we merge this

frasercrmck

Generally LGTM. I've the same old quibbles about manually mangling things like types and address spaces. I hope we can move to C++ to define these builtins sooner rather than later. Not for this patch, though.

frasercrmck · 2025-02-06T14:55:16Z

libclc/libspirv/lib/amdgcn-amdhsa/images/image_common.h

+// defined in "image_common.cl"
+extern _CLC_CONST_AS const unsigned int SAMPLER_OBJECT_OFFSET_DWORD;
+
+// Helpers for casting between two builitin vector types and/or scalar types.


typo: builitin -> builtin (happens to me all the time)

GeorgeWeb · 2025-02-06T15:09:47Z

I'd like to get @frasercrmck's opinion on the libclc changes before we merge this

Thanks for double checking, @ldrumm. I should’ve also requested Fraser to take a look knowing he started improving libclc->libspirv as a whole.
Appreciated!

Signed-off-by: JackAKirk <[email protected]>

ldrumm · 2025-02-06T15:23:00Z

Not waiting for a full CI run on a comment change. This looks good to go

This PR implements the entry points in the HIP adapter for the Bindless Images extension. Some features, while implemented, only partially pass the End-to-end tests for them in DPC++ (the PR that enables the support and testing in DPC++: #16439). This is because the HIP runtime doesn't support, for example, some parameter combinations with some image memory types. The not-fully supported features are marked with TODOs in the device info queries for further work. Signed-off-by: Georgi Mirazchiyski <[email protected]> Co-authored-by: Peter Žužek <[email protected]>

GeorgeWeb had a problem deploying to WindowsCILock December 20, 2024 13:37 — with GitHub Actions Failure

GeorgeWeb force-pushed the georgi/bindless-hip branch from 9384f74 to 437f972 Compare December 20, 2024 13:46

GeorgeWeb temporarily deployed to WindowsCILock December 20, 2024 13:46 — with GitHub Actions Inactive

GeorgeWeb temporarily deployed to WindowsCILock December 20, 2024 14:15 — with GitHub Actions Inactive

GeorgeWeb mentioned this pull request Dec 23, 2024

[UR][Bindless] Initial implementation of bindless images for HIP oneapi-src/unified-runtime#2496

Merged

GeorgeWeb marked this pull request as ready for review December 23, 2024 15:17

GeorgeWeb requested review from a team as code owners December 23, 2024 15:17

GeorgeWeb requested a review from jchlanda December 23, 2024 15:17

GeorgeWeb added the sycl-bindless-images SYCL Bindless Images label Dec 23, 2024

jchlanda reviewed Jan 8, 2025

View reviewed changes

jchlanda approved these changes Jan 8, 2025

View reviewed changes

DBDuncan reviewed Jan 20, 2025

View reviewed changes

sycl/test-e2e/bindless_images/array/read_write_1d_subregion.cpp Outdated Show resolved Hide resolved

GeorgeWeb force-pushed the georgi/bindless-hip branch from eadd947 to e5e0d35 Compare January 23, 2025 18:14

DBDuncan reviewed Jan 24, 2025

View reviewed changes

sycl/test-e2e/bindless_images/read_sampled.cpp Outdated Show resolved Hide resolved

GeorgeWeb force-pushed the georgi/bindless-hip branch from e5e0d35 to 3d7280b Compare January 27, 2025 12:13

GeorgeWeb force-pushed the georgi/bindless-hip branch from 3d7280b to 0769128 Compare January 27, 2025 12:20

GeorgeWeb had a problem deploying to WindowsCILock January 27, 2025 12:22 — with GitHub Actions Failure

GeorgeWeb had a problem deploying to WindowsCILock January 27, 2025 12:51 — with GitHub Actions Failure

GeorgeWeb added 3 commits January 29, 2025 16:50

Split image.cl into image, image_array and image_common

e83ea79

Signed-off-by: Georgi Mirazchiyski <[email protected]>

Fix and cleanup some tests

729dd32

Signed-off-by: Georgi Mirazchiyski <[email protected]>

GeorgeWeb force-pushed the georgi/bindless-hip branch from 0769128 to 729dd32 Compare January 29, 2025 16:51

GeorgeWeb temporarily deployed to WindowsCILock January 29, 2025 16:53 — with GitHub Actions Inactive

GeorgeWeb had a problem deploying to WindowsCILock January 29, 2025 18:09 — with GitHub Actions Failure

JackAKirk requested a review from a team as a code owner February 5, 2025 16:59

JackAKirk requested a review from cperkinsintel February 5, 2025 16:59

JackAKirk had a problem deploying to WindowsCILock February 5, 2025 17:01 — with GitHub Actions Failure

JackAKirk marked this pull request as draft February 5, 2025 17:01

use run-unfiltered-devices to fix tests.

b737bfa

Signed-off-by: JackAKirk <[email protected]>

JackAKirk marked this pull request as ready for review February 5, 2025 17:38

JackAKirk temporarily deployed to WindowsCILock February 5, 2025 17:38 — with GitHub Actions Inactive

JackAKirk temporarily deployed to WindowsCILock February 5, 2025 18:14 — with GitHub Actions Inactive

Try build-and-run-mode

3207b6d

Signed-off-by: JackAKirk <[email protected]>

JackAKirk temporarily deployed to WindowsCILock February 6, 2025 10:14 — with GitHub Actions Inactive

JackAKirk had a problem deploying to WindowsCILock February 6, 2025 10:49 — with GitHub Actions Error

Fix broken SPIR cubemap compilation

203d1e8

Signed-off-by: JackAKirk <[email protected]>

JackAKirk temporarily deployed to WindowsCILock February 6, 2025 11:21 — with GitHub Actions Inactive

JackAKirk had a problem deploying to WindowsCILock February 6, 2025 11:49 — with GitHub Actions Error

use build-and-run-mode sampled_cubemap.cpp

f67dd1c

Signed-off-by: JackAKirk <[email protected]>

JackAKirk temporarily deployed to WindowsCILock February 6, 2025 12:16 — with GitHub Actions Inactive

JackAKirk temporarily deployed to WindowsCILock February 6, 2025 12:44 — with GitHub Actions Inactive

kbenzie mentioned this pull request Feb 6, 2025

[SYCL][NativeCPU] Handle null phEvent. #16868

Merged

frasercrmck approved these changes Feb 6, 2025

View reviewed changes

Fix comment typo

d78fdfe

Signed-off-by: JackAKirk <[email protected]>

JackAKirk temporarily deployed to WindowsCILock February 6, 2025 15:20 — with GitHub Actions Inactive

ldrumm merged commit 3161af3 into intel:sycl Feb 6, 2025
6 checks passed

JackAKirk temporarily deployed to WindowsCILock February 6, 2025 15:50 — with GitHub Actions Inactive

przemektmalon mentioned this pull request Feb 25, 2025

[SYCL][E2E] Clean-up test-modes by removing workarounds #17023

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL][Ext][Bindless] Initial implementation of image spirv builtins on HIP #16439

[SYCL][Ext][Bindless] Initial implementation of image spirv builtins on HIP #16439

GeorgeWeb commented Dec 20, 2024 •

edited

Loading

GeorgeWeb commented Jan 7, 2025 •

edited

Loading

jchlanda left a comment

jchlanda Jan 7, 2025

GeorgeWeb Jan 23, 2025

jchlanda Jan 27, 2025

GeorgeWeb Jan 27, 2025

jchlanda Jan 27, 2025

jchlanda commented Jan 8, 2025

JackAKirk commented Feb 5, 2025

JackAKirk commented Feb 5, 2025

ldrumm commented Feb 6, 2025

frasercrmck left a comment

frasercrmck Feb 6, 2025

GeorgeWeb commented Feb 6, 2025 •

edited

Loading

ldrumm commented Feb 6, 2025

[SYCL][Ext][Bindless] Initial implementation of image spirv builtins on HIP #16439

[SYCL][Ext][Bindless] Initial implementation of image spirv builtins on HIP #16439

Conversation

GeorgeWeb commented Dec 20, 2024 • edited Loading

GeorgeWeb commented Jan 7, 2025 • edited Loading

jchlanda left a comment

Choose a reason for hiding this comment

jchlanda Jan 7, 2025

Choose a reason for hiding this comment

GeorgeWeb Jan 23, 2025

Choose a reason for hiding this comment

jchlanda Jan 27, 2025

Choose a reason for hiding this comment

GeorgeWeb Jan 27, 2025

Choose a reason for hiding this comment

jchlanda Jan 27, 2025

Choose a reason for hiding this comment

jchlanda commented Jan 8, 2025

JackAKirk commented Feb 5, 2025

JackAKirk commented Feb 5, 2025

ldrumm commented Feb 6, 2025

frasercrmck left a comment

Choose a reason for hiding this comment

frasercrmck Feb 6, 2025

Choose a reason for hiding this comment

GeorgeWeb commented Feb 6, 2025 • edited Loading

ldrumm commented Feb 6, 2025

GeorgeWeb commented Dec 20, 2024 •

edited

Loading

GeorgeWeb commented Jan 7, 2025 •

edited

Loading

GeorgeWeb commented Feb 6, 2025 •

edited

Loading