Skip to content

Conversation

@hdelan
Copy link
Contributor

@hdelan hdelan commented Jul 31, 2024

Now that level zero supports this extension as of
oneapi-src/unified-runtime#1880, this extension should be a oneapi
ext instead of a codeplay one.

Now that level zero supports this extension, it should be a oneapi ext
instead of a codeplay one.
@hdelan hdelan requested review from a team as code owners July 31, 2024 09:26
@hdelan hdelan requested a review from aelovikov-intel July 31, 2024 09:26
@EwanC
Copy link
Contributor

EwanC commented Jul 31, 2024

Should this bump the UR L0 adapter tag to oneapi-src/unified-runtime#1880 ?

@hdelan
Copy link
Contributor Author

hdelan commented Jul 31, 2024

Should this bump the UR L0 adapter tag to oneapi-src/unified-runtime#1880 ?

Yeah spot on actually will do so now

@hdelan hdelan requested a review from a team as a code owner July 31, 2024 09:53
@hdelan hdelan temporarily deployed to WindowsCILock July 31, 2024 10:37 — with GitHub Actions Inactive
@hdelan hdelan temporarily deployed to WindowsCILock July 31, 2024 11:09 — with GitHub Actions Inactive
@hdelan hdelan force-pushed the codeplay-to-oneapi-ext branch from 75edd53 to 169510e Compare July 31, 2024 13:24
@hdelan hdelan temporarily deployed to WindowsCILock July 31, 2024 13:25 — with GitHub Actions Inactive
@gmlueck
Copy link
Contributor

gmlueck commented Jul 31, 2024

I think we should have the following before we rename:

  • e2e tests for Level Zero
  • Expand the "Examples" section in the specification to include a Level Zero example. Make that the first example.
  • Change the example in "Overview" that illustrates the purpose of this extension to use Level Zero instead of HIP.

@hdelan hdelan temporarily deployed to WindowsCILock July 31, 2024 14:15 — with GitHub Actions Inactive
@omarahmed1111
Copy link
Contributor

The bump for this PR will be included as part of this PR, so we could remove the UR bump here.

@hdelan
Copy link
Contributor Author

hdelan commented Jul 31, 2024

The bump for this PR will be included as part of this PR, so we could remove the UR bump here.

Good shout, when that PR gets merged I'll rebase and get rid of the bump as part of this PR.

@hdelan
Copy link
Contributor Author

hdelan commented Jul 31, 2024

I think we should have the following before we rename:

  • e2e tests for Level Zero
  • Expand the "Examples" section in the specification to include a Level Zero example. Make that the first example.
  • Change the example in "Overview" that illustrates the purpose of this extension to use Level Zero instead of HIP.

Changes made. Let me know what you think

Use L0 build options.
@aelovikov-intel
Copy link
Contributor

Please change PR's title to name the extension, instead of just "codeplay extension" that can be misleading.

@hdelan hdelan changed the title [NFC] Rename codeplay extension to oneapi extension [NFC] Rename ext_codeplay_enqueue_native_command ext_oneapi_enqueue_native_command Jul 31, 2024
@hdelan
Copy link
Contributor Author

hdelan commented Jul 31, 2024

Please change PR's title to name the extension, instead of just "codeplay extension" that can be misleading.

Change made. Thanks

@hdelan hdelan changed the title [NFC] Rename ext_codeplay_enqueue_native_command ext_oneapi_enqueue_native_command [NFC] Rename ext_codeplay_enqueue_native_command to ext_oneapi_enqueue_native_command Jul 31, 2024
auto DstMem = IH.get_native_mem<backend::ext_oneapi_level_zero>(DstA);

// If L0 interop becomes a real use case we should make a new UR entry
// point to propagate events into and out of the the interop func.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this comment mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to make this entry point more performant on L0 we would need something like the previous host task extensions so we can pass in and out events to the ze call.

As it stands there is no point making this more performant for L0 since no one that we are aware of are using L0 interop in SYCL.

Copy link
Contributor

Choose a reason for hiding this comment

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

????

We have had a SYCL Level Zero interop specification for a long time and many people are using it.

Use accessor::size instead of get_count.
Use accessors to show where src and dst come from and use accessor::size
instead of accessor::get_count. Also update comment to say command list
instead of stream.
@hdelan
Copy link
Contributor Author

hdelan commented Aug 7, 2024

Closing as no longer required

@hdelan hdelan closed this Aug 7, 2024
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.

5 participants