[SYCL][Bindless][2/4] Add experimental implementation of SYCL bindless images extension #10112

isaacault · 2023-06-28T13:25:47Z

Experimental Implementation of SYCL Bindless Images Extension

This commit stands as the second commit of four to make code review easier, implementing revision 4 of the bindless images extension proposal.

Scope

This PR covers changes made to the PI and the UR. This includes

Extending PI with extension functions
Updating UR FetchContent commit and implementing UR bindless images experimental features on the CUDA adaptor

Following Split PRs

[3/4] Implement the user-facing SYCL extension
[4/4] Add tests

Authors

Co-authored-by: Isaac Ault isaac.ault@codeplay.com
Co-authored-by: Hugh Bird hugh.bird@codeplay.com
Co-authored-by: Duncan Brawley duncan.brawley@codeplay.com
Co-authored-by: Przemek Malon przemek.malon@codeplay.com
Co-authored-by: Chedy Najjar chedy.najjar@codeplay.com
Co-authored-by: Sean Stirling sean.stirling@codeplay.com
Co-authored-by: Peter Zuzek peter@codeplay.com

…s images extension This commit stands as the second commit of four to make code review easier, covering the changes made to the PI. Co-authored-by: Isaac Ault <isaac.ault@codeplay.com> Co-authored-by: Hugh Bird <hugh.bird@codeplay.com> Co-authored-by: Duncan Brawley <duncan.brawley@codeplay.com> Co-authored-by: Przemek Malon <przemek.malon@codeplay.com> Co-authored-by: Chedy Najjar <chedy.najjar@codeplay.com> Co-authored-by: Sean Stirling <sean.stirling@codeplay.com> Co-authored-by: Peter Zuzek <peter@codeplay.com> Implement revision 4 of the bindless images extension proposal: intel#9842

sycl/test/abi/sycl_symbols_linux.dump

sycl/plugins/level_zero/pi_level_zero.cpp

aelovikov-intel · 2023-06-28T14:53:56Z

sycl/plugins/unified_runtime/pi2ur.hpp

+  case UR_IMAGE_CHANNEL_ORDER_A: {
+    PiFormat->image_channel_order = PI_IMAGE_CHANNEL_ORDER_A;
+    break;
+  }


How about something like

#define MAP(FROM, TO) \ case FROM: { \ PiFormat->image_channel_order = TO; \ break; \ }

And if we can change the spelling for enum entries in cases like

case UR_IMAGE_CHANNEL_ORDER_RX: { PiFormat->image_channel_order = PI_IMAGE_CHANNEL_ORDER_Rx;

then even FROM/TO could be replaced with a single argument...

Added a MAP macro as you've specified. One for each of channel_order and channel_data_type.

While we are at it, the helpers could also be done in the MAP manner, for example pi2urImageDesc.

@aelovikov-intel @jchlanda Done.

aelovikov-intel · 2023-06-28T14:55:34Z

sycl/plugins/unified_runtime/ur/adapters/cuda/device.cpp

@@ -873,6 +873,85 @@ UR_APIEXPORT ur_result_t UR_APICALL urDeviceGetInfo(ur_device_handle_t hDevice,
  case UR_DEVICE_INFO_MAX_COMPUTE_QUEUE_INDICES: {
    return ReturnValue(int32_t{1});
  }
+  case UR_DEVICE_INFO_BINDLESS_IMAGES_SUPPORT_EXP: {
+    // On CUDA bindless images are supported


Suggested change

// On CUDA bindless images are supported

// On CUDA bindless images are supported.

Same elsewhere.

Added punctuation to comments throughout.

* [[maybe_unused]] flag added to pi_level_zero * pi_hip piSamplerCreate updated for API change * Macro used for image format switch cases * Added punctuation to comments

aelovikov-intel · 2023-06-29T14:33:51Z

sycl/plugins/unified_runtime/pi2ur.hpp

+  case PI_EXT_ONEAPI_DEVICE_INFO_BINDLESS_IMAGES_SUPPORT: {
+    InfoType = UR_DEVICE_INFO_BINDLESS_IMAGES_SUPPORT_EXP;
+    break;
+  }


Can use MAP here as well.

Done. Applied to add cases for this statement.

aelovikov-intel · 2023-06-29T14:35:44Z

sycl/plugins/unified_runtime/ur/adapters/cuda/image.cpp

+  case UR_IMAGE_CHANNEL_TYPE_UNORM_INT8:
+    cuda_format = CU_AD_FORMAT_UNORM_INT8X1;
+    PixelTypeSizeBytes = 1;
+    break;


Three-operand MAP/CASE macro?

aelovikov-intel · 2023-06-29T14:36:05Z

sycl/plugins/unified_runtime/ur/adapters/cuda/image.cpp

+  case CU_AD_FORMAT_UNSIGNED_INT8:
+    *return_image_channel_type = UR_IMAGE_CHANNEL_TYPE_UNSIGNED_INT8;
+    break;


przemektmalon · 2023-07-17T11:23:16Z

@jandres742 We have incorporated the latest UR changes related to interop resource handles. These are now represented by structs and passed to the UR through pNext chains, instead of the explicit int types that were passed previously.

Can you confirm whether you are happy with the changes, and if so, could you please give your approval on this PR?

jandres742 · 2023-07-18T03:57:00Z

@jandres742 We have incorporated the latest UR changes related to interop resource handles. These are now represented by structs and passed to the UR through pNext chains, instead of the explicit int types that were passed previously.

Can you confirm whether you are happy with the changes, and if so, could you please give your approval on this PR?

Thanks @przemektmalon .

I think we need to define the export API. Even if there were no support, we are defining here the extension, so it should include the export APIs, so whenever support is added to the backends, the SYCL and PI support is already there.

what do you think? does it make sense?

jbrodman · 2023-07-18T15:45:59Z

@jandres742 We have incorporated the latest UR changes related to interop resource handles. These are now represented by structs and passed to the UR through pNext chains, instead of the explicit int types that were passed previously.
Can you confirm whether you are happy with the changes, and if so, could you please give your approval on this PR?

Thanks @przemektmalon .

I think we need to define the export API. Even if there were no support, we are defining here the extension, so it should include the export APIs, so whenever support is added to the backends, the SYCL and PI support is already there.

what do you think? does it make sense?

Let's defer additions to future PRs so we can unblock this. It is now ready to merge, correct?

Ruyk · 2023-07-18T15:53:00Z

This is OK from the CP team perspective, we have addressing all comments and concerns.

aelovikov-intel · 2023-07-18T20:18:08Z

It seems this broke post-commit: https://github.com/intel/llvm/actions/runs/5589657189/jobs/10218527948.

jandres742 · 2023-07-18T23:31:49Z

@jandres742 We have incorporated the latest UR changes related to interop resource handles. These are now represented by structs and passed to the UR through pNext chains, instead of the explicit int types that were passed previously.
Can you confirm whether you are happy with the changes, and if so, could you please give your approval on this PR?

Thanks @przemektmalon .
I think we need to define the export API. Even if there were no support, we are defining here the extension, so it should include the export APIs, so whenever support is added to the backends, the SYCL and PI support is already there.
what do you think? does it make sense?

Let's defer additions to future PRs so we can unblock this. It is now ready to merge, correct?

Thanks @jbrodman . Sure, if the API to export will be added shortly after, no concerns.

steffenlarsen · 2023-07-19T08:54:17Z

It seems this broke post-commit: https://github.com/intel/llvm/actions/runs/5589657189/jobs/10218527948.

@isaacault | @przemektmalon - Could you please address the post-commit failure ASAP?

- Fixed compiler errors/warnings related to unused and uninitialized variables and parameters. Post-commit fix for PR: intel#10112

przemektmalon · 2023-07-19T11:20:11Z

It seems this broke post-commit: https://github.com/intel/llvm/actions/runs/5589657189/jobs/10218527948.

@isaacault | @przemektmalon - Could you please address the post-commit failure ASAP?

Created PR 10464 to address this.

- Fixed compiler errors/warnings related to unused and uninitialized variables and parameters. Post-commit fix for PR: #10112

…CL bindless images extension (#10500) # Experimental Implementation of SYCL Bindless Images Extension This commit stands as the fourth, and final, commit of four to make code review easier, covering the additional tests for bindless images to the e2e test suite. Implementing [revision 4 of the bindless images extension proposal](#9842). This will not compile or run until [PR3](#10454) has been merged. However, it can be reviewed simultaneously with PR3. ## Overview The bindless images extension provides a new interface for allocating, creating, and accessing images in SYCL. Image memory allocation is seperated from image handle creation, and image handles can be passed to kernels without requesting access through accessors. This approach provides much more flexibility to the user, as well as enabling programs to implement features that were impossible to implement using standard SYCL images, such as a texture atlas. In addition to providing a new interface for images, this extension also provides initial experimental support for importing external memory into SYCL. ## Previous PRs * [1/4] [libclc](#9808) * [2/4] [PI/UR](#10112) * [3/4] [SYCL API](#10454) * [4/4] Tests <--- This one ## Authors Co-authored-by: Isaac Ault isaac.ault@codeplay.com Co-authored-by: Hugh Bird hugh.bird@codeplay.com Co-authored-by: Duncan Brawley duncan.brawley@codeplay.com Co-authored-by: Przemek Malon przemek.malon@codeplay.com Co-authored-by: Chedy Najjar chedy.najjar@codeplay.com Co-authored-by: Sean Stirling sean.stirling@codeplay.com Co-authored-by: Peter Zuzek peter@codeplay.com Co-authored-by: SYCL Unbound Team <sycl.unbound@codeplay.com>

…s images extension (intel#10112) # Experimental Implementation of SYCL Bindless Images Extension This commit stands as the second commit of four to make code review easier, implementing revision 4 of the [bindless images extension proposal](intel#9842). ## Scope This PR covers changes made to the PI and the UR. This includes - Extending PI with extension functions - Updating UR FetchContent commit and implementing [UR bindless images experimental features](https://oneapi-src.github.io/unified-runtime/core/EXP-BINDLESS-IMAGES.html) on the CUDA adaptor ## Following Split PRs - [3/4] Implement the user-facing SYCL extension - [4/4] Add tests ## Authors Co-authored-by: Isaac Ault <isaac.ault@codeplay.com> Co-authored-by: Hugh Bird <hugh.bird@codeplay.com> Co-authored-by: Duncan Brawley <duncan.brawley@codeplay.com> Co-authored-by: Przemek Malon <przemek.malon@codeplay.com> Co-authored-by: Chedy Najjar <chedy.najjar@codeplay.com> Co-authored-by: Sean Stirling <sean.stirling@codeplay.com> Co-authored-by: Peter Zuzek <peter@codeplay.com>

- Fixed compiler errors/warnings related to unused and uninitialized variables and parameters. Post-commit fix for PR: intel#10112

- Fixed compiler errors/warnings related to unused and uninitialized variables and parameters. Post-commit fix for PR: intel/llvm#10112

…CL bindless images extension (intel#10500) # Experimental Implementation of SYCL Bindless Images Extension This commit stands as the fourth, and final, commit of four to make code review easier, covering the additional tests for bindless images to the e2e test suite. Implementing [revision 4 of the bindless images extension proposal](intel#9842). This will not compile or run until [PR3](intel#10454) has been merged. However, it can be reviewed simultaneously with PR3. ## Overview The bindless images extension provides a new interface for allocating, creating, and accessing images in SYCL. Image memory allocation is seperated from image handle creation, and image handles can be passed to kernels without requesting access through accessors. This approach provides much more flexibility to the user, as well as enabling programs to implement features that were impossible to implement using standard SYCL images, such as a texture atlas. In addition to providing a new interface for images, this extension also provides initial experimental support for importing external memory into SYCL. ## Previous PRs * [1/4] [libclc](intel#9808) * [2/4] [PI/UR](intel#10112) * [3/4] [SYCL API](intel#10454) * [4/4] Tests <--- This one ## Authors Co-authored-by: Isaac Ault isaac.ault@codeplay.com Co-authored-by: Hugh Bird hugh.bird@codeplay.com Co-authored-by: Duncan Brawley duncan.brawley@codeplay.com Co-authored-by: Przemek Malon przemek.malon@codeplay.com Co-authored-by: Chedy Najjar chedy.najjar@codeplay.com Co-authored-by: Sean Stirling sean.stirling@codeplay.com Co-authored-by: Peter Zuzek peter@codeplay.com Co-authored-by: SYCL Unbound Team <sycl.unbound@codeplay.com>

- Fixed compiler errors/warnings related to unused and uninitialized variables and parameters. Post-commit fix for PR: intel/llvm#10112

isaacault requested review from a team as code owners June 28, 2023 13:25

isaacault requested review from jchlanda and aelovikov-intel June 28, 2023 13:25

isaacault force-pushed the codeplay/bindless_images_pi_ur branch 2 times, most recently from f74cf46 to a450cd7 Compare June 28, 2023 13:52

isaacault temporarily deployed to aws June 28, 2023 13:57 — with GitHub Actions Inactive

isaacault force-pushed the codeplay/bindless_images_pi_ur branch from a450cd7 to fc60063 Compare June 28, 2023 14:05

isaacault temporarily deployed to aws June 28, 2023 14:10 — with GitHub Actions Inactive

steffenlarsen reviewed Jun 28, 2023

View reviewed changes

sycl/test/abi/sycl_symbols_linux.dump Outdated Show resolved Hide resolved

aelovikov-intel reviewed Jun 28, 2023

View reviewed changes

sycl/plugins/level_zero/pi_level_zero.cpp Outdated Show resolved Hide resolved

aelovikov-intel reviewed Jun 28, 2023

View reviewed changes

isaacault changed the title ~~[SYCL][Bindless][2/4] Add experimental implementation of SYCL bindless images extension~~ [SYCL][Bindless][ABI-break][2/4] Add experimental implementation of SYCL bindless images extension Jun 29, 2023

isaacault force-pushed the codeplay/bindless_images_pi_ur branch 2 times, most recently from 5aa5702 to 082a6f4 Compare June 29, 2023 10:55

Address Feedback

109414b

* [[maybe_unused]] flag added to pi_level_zero * pi_hip piSamplerCreate updated for API change * Macro used for image format switch cases * Added punctuation to comments

isaacault force-pushed the codeplay/bindless_images_pi_ur branch from 082a6f4 to 109414b Compare June 29, 2023 11:05

isaacault temporarily deployed to aws June 29, 2023 12:14 — with GitHub Actions Inactive

Add piext functions to pi_esimd_emulator

f4b06f2

isaacault temporarily deployed to aws June 29, 2023 13:41 — with GitHub Actions Inactive

isaacault force-pushed the codeplay/bindless_images_pi_ur branch from d0c3261 to 07320a2 Compare June 29, 2023 13:53

Update Level Zero symbol dump file

031107f

isaacault force-pushed the codeplay/bindless_images_pi_ur branch from 07320a2 to 031107f Compare June 29, 2023 13:54

aelovikov-intel reviewed Jun 29, 2023

View reviewed changes

isaacault temporarily deployed to aws June 29, 2023 14:39 — with GitHub Actions Inactive

ProGTX mentioned this pull request Jun 29, 2023

[SYCL][Doc] Bindless Images proposal revision 4 #9842

Merged

Remove generated files

28240c9

przemektmalon temporarily deployed to aws July 14, 2023 16:58 — with GitHub Actions Inactive

przemektmalon temporarily deployed to aws July 14, 2023 18:18 — with GitHub Actions Inactive

przemektmalon added 3 commits July 17, 2023 10:32

Merge branch 'sycl' into codeplay/bindless_images_pi_ur

f671af6

Fix assertion in cuda device.cpp

97be70a

Merge branch 'sycl' into codeplay/bindless_images_pi_ur

0a5a2e8

przemektmalon temporarily deployed to aws July 17, 2023 10:26 — with GitHub Actions Inactive

przemektmalon temporarily deployed to aws July 17, 2023 11:53 — with GitHub Actions Inactive

przemektmalon mentioned this pull request Jul 18, 2023

[SYCL][Bindless][3/4] Add experimental implementation of SYCL bindless images extension #10454

Merged

steffenlarsen merged commit 380453d into intel:sycl Jul 18, 2023

przemektmalon added a commit to codeplaysoftware/intel-llvm-mirror that referenced this pull request Jul 19, 2023

[SYCL][Bindless] Fix compiler Werrors

3fffb97

- Fixed compiler errors/warnings related to unused and uninitialized variables and parameters. Post-commit fix for PR: intel#10112

przemektmalon mentioned this pull request Jul 19, 2023

[SYCL][Bindless] Fix compiler Werrors #10464

Merged

dm-vodopyanov pushed a commit that referenced this pull request Jul 19, 2023

[SYCL][Bindless] Fix compiler Werrors (#10464)

6f32df0

- Fixed compiler errors/warnings related to unused and uninitialized variables and parameters. Post-commit fix for PR: #10112

Seanst98 mentioned this pull request Jul 20, 2023

[SYCL][Bindless][4/4] Add tests for experimental implementation of SYCL bindless images extension #10500

Merged

fabiomestre pushed a commit to fabiomestre/llvm that referenced this pull request Sep 26, 2023

[SYCL][Bindless] Fix compiler Werrors (intel#10464)

ea207a0

- Fixed compiler errors/warnings related to unused and uninitialized variables and parameters. Post-commit fix for PR: intel#10112

	// On CUDA bindless images are supported
	// On CUDA bindless images are supported.

[SYCL][Bindless][2/4] Add experimental implementation of SYCL bindless images extension #10112

[SYCL][Bindless][2/4] Add experimental implementation of SYCL bindless images extension #10112

Uh oh!

Conversation

isaacault commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Experimental Implementation of SYCL Bindless Images Extension

Scope

Following Split PRs

Authors

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

przemektmalon commented Jul 17, 2023

Uh oh!

jandres742 commented Jul 18, 2023

Uh oh!

jbrodman commented Jul 18, 2023

Uh oh!

Ruyk commented Jul 18, 2023

Uh oh!

aelovikov-intel commented Jul 18, 2023

Uh oh!

jandres742 commented Jul 18, 2023

Uh oh!

steffenlarsen commented Jul 19, 2023

Uh oh!

przemektmalon commented Jul 19, 2023

Uh oh!

Uh oh!

isaacault commented Jun 28, 2023 •

edited

Loading