Skip to content

Conversation

@rdeodhar
Copy link
Contributor

@rdeodhar rdeodhar commented May 3, 2023

This change adds a SYCL interface to the Level Zero APIs zexDriverImportExternalPointer and zexDriverReleaseImportedPointer. These functions are used for importing host memory into USM for the duration of data transfer to increase bandwidth.

@rdeodhar rdeodhar temporarily deployed to aws May 3, 2023 20:18 — with GitHub Actions Inactive
@rdeodhar rdeodhar temporarily deployed to aws May 5, 2023 02:28 — with GitHub Actions Inactive
@rdeodhar rdeodhar temporarily deployed to aws May 5, 2023 16:07 — with GitHub Actions Inactive
@rdeodhar rdeodhar temporarily deployed to aws May 5, 2023 23:06 — with GitHub Actions Inactive
@rdeodhar rdeodhar temporarily deployed to aws May 6, 2023 03:52 — with GitHub Actions Inactive
@rdeodhar rdeodhar marked this pull request as ready for review May 6, 2023 20:20
@rdeodhar rdeodhar requested review from a team as code owners May 6, 2023 20:20
@rdeodhar rdeodhar requested review from bso-intel and jchlanda May 6, 2023 20:20
@jchlanda
Copy link
Contributor

jchlanda commented May 11, 2023

CUDA and HIP LGTM.

However, I wonder if this has to be a oneapi-wide extension, would it not be better suited as sycl_ext_intel_...? It is a noop for all but L0 backends and seems unlikely that it will ever change.
Alternatively, would it be possible, instead of creating new PI entries, have logic in prepare_for_device_copy and friends that dispatches to correct L0 free functions?

@rdeodhar
Copy link
Contributor Author

Although the implementation has been fleshed out for L0, the intent is to allow other platforms to do what is necessary to accelerate host<->device transfers within the range of the prepare / release.

@rdeodhar rdeodhar temporarily deployed to aws May 19, 2023 20:43 — with GitHub Actions Inactive
return result;
}

pi_result piextUSMImport(void *ptr, size_t size, pi_context context) {
Copy link
Contributor

@jandres742 jandres742 May 19, 2023

Choose a reason for hiding this comment

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

we have something similar in CU, cudaHostRegister, cudaHostUnregister . should we expand the definition of the extension to cover the functionality offered by all backends?

Moreover, the L0 APIs are driver-experimental extensions, and when merged to the spec, they would likely be much different. i dont think that we should define a SYCL extension until the L0 interfaces are at least defined in the L0 spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not quite equivalent to the cuda HostRegister APIs. Those make the address range accessible from host and device and in addition pin the host memory to physical memory. We should add "pinning" as a separate extension when it is supported by L0.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks @rdeodhar . Now that we are moving to the UR, all PI interfaces should be designed taking into account all interfaces. Or maybe, something like UR extensions would help here. I will bring up this in the UR WGs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Moreover, the L0 APIs are driver-experimental extensions, and when merged to the spec, they would likely be much different. i dont think that we should define a SYCL extension until the L0 interfaces are at least defined in the L0 spec.

Note that this is also an experimental SYCL extension. We can replace it with a more general API later if the L0 API changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gmlueck : Even if this is an experimental extension, we need UR interfaces for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. I was only responding to your comment that I quoted, where you seemed to be implying that we should not have SYCL APIs that expose experimental L0 features.

Copy link
Contributor

@alycm alycm May 30, 2023

Choose a reason for hiding this comment

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

@rdeodhar Documentation on how to support experimental features is still being written by @kbenzie, but the draft PR is here: oneapi-src/unified-runtime#546

As this is experimental the UR experimental feature can look identical to the PI feature if that is the quickest way to keep this PR moving.

You can also ask us (@alycm, @jandres742, @kbenzie) any questions and we'll help you out.

@rdeodhar rdeodhar temporarily deployed to aws July 12, 2023 22:24 — with GitHub Actions Inactive
@rdeodhar rdeodhar temporarily deployed to aws July 12, 2023 22:24 — with GitHub Actions Inactive
@rdeodhar rdeodhar temporarily deployed to aws July 13, 2023 15:33 — with GitHub Actions Inactive
@rdeodhar rdeodhar temporarily deployed to aws July 13, 2023 15:35 — with GitHub Actions Inactive
@rdeodhar rdeodhar temporarily deployed to aws July 13, 2023 17:11 — with GitHub Actions Inactive
@rdeodhar rdeodhar temporarily deployed to aws July 13, 2023 22:23 — with GitHub Actions Inactive
@rdeodhar rdeodhar temporarily deployed to aws July 14, 2023 18:21 — with GitHub Actions Inactive
@rdeodhar rdeodhar temporarily deployed to aws July 15, 2023 00:28 — with GitHub Actions Inactive
@rdeodhar rdeodhar temporarily deployed to aws July 17, 2023 21:08 — with GitHub Actions Inactive
@rdeodhar rdeodhar temporarily deployed to aws July 18, 2023 17:57 — with GitHub Actions Inactive
@rdeodhar rdeodhar temporarily deployed to aws July 18, 2023 19:00 — with GitHub Actions Inactive
@rdeodhar rdeodhar temporarily deployed to aws July 18, 2023 20:20 — with GitHub Actions Inactive
@rdeodhar rdeodhar temporarily deployed to aws July 18, 2023 21:52 — with GitHub Actions Inactive
@rdeodhar rdeodhar temporarily deployed to aws July 19, 2023 04:11 — with GitHub Actions Inactive
@rdeodhar rdeodhar temporarily deployed to aws July 19, 2023 04:19 — with GitHub Actions Inactive
@againull againull merged commit 65cc0cf into intel:sycl Jul 19, 2023
@dm-vodopyanov
Copy link
Contributor

Failure in post-commit: build failed on Linux and macOS.
@rdeodhar could you please fix ASAP?

veselypeta pushed a commit to veselypeta/llvm that referenced this pull request Sep 21, 2023
…ntel#9294)

This change adds a SYCL interface to the Level Zero APIs
zexDriverImportExternalPointer and zexDriverReleaseImportedPointer.
These functions are used for importing host memory into USM for the
duration of data transfer to increase bandwidth.
fabiomestre pushed a commit to fabiomestre/llvm that referenced this pull request Sep 26, 2023
…ntel#9294)

This change adds a SYCL interface to the Level Zero APIs
zexDriverImportExternalPointer and zexDriverReleaseImportedPointer.
These functions are used for importing host memory into USM for the
duration of data transfer to increase bandwidth.
veselypeta pushed a commit to veselypeta/llvm that referenced this pull request Sep 28, 2023
…ntel#9294)

This change adds a SYCL interface to the Level Zero APIs
zexDriverImportExternalPointer and zexDriverReleaseImportedPointer.
These functions are used for importing host memory into USM for the
duration of data transfer to increase bandwidth.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants