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.

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

[SYCL][CUDA] Pass device from context in create queue. #10491

Merged
merged 2 commits into from
Aug 4, 2023

Conversation

JackAKirk
Copy link
Contributor

@JackAKirk JackAKirk commented Jul 20, 2023

Recently in the switch to UR urQueueCreateFromNativeHandle changed the previous behaviour whereby a queue was created with a device taken as the default device from the context. It changed it so that the queue was created with the device argument instead. Since the sycl runtime always passes a nullptr for the device when programmers call make_queue(nativeStream, context), this broke make_queue. This patch reverts to the previous behaviour before the switch from pi cuda to ur cuda.

Note that this should also fix make_queue for l0 which I also guess was broken due to the asserts meaning that this line was never reached: https://github.com/intel/llvm/blob/sycl/sycl/plugins/unified_runtime/ur/adapters/level_zero/queue.cpp#L574. But I have not tested this.

@JackAKirk JackAKirk requested review from a team as code owners July 20, 2023 10:22
@JackAKirk JackAKirk requested a review from jchlanda July 20, 2023 10:22
@JackAKirk JackAKirk temporarily deployed to aws July 20, 2023 10:37 — with GitHub Actions Inactive
@JackAKirk JackAKirk temporarily deployed to aws July 20, 2023 11:17 — with GitHub Actions Inactive
Signed-off-by: JackAKirk <[email protected]>
@JackAKirk JackAKirk temporarily deployed to aws July 20, 2023 13:06 — with GitHub Actions Inactive
@JackAKirk JackAKirk temporarily deployed to aws July 20, 2023 13:45 — with GitHub Actions Inactive
@jandres742
Copy link
Contributor

Since the sycl runtime always passes a nullptr for the device when programmers call make_queue(nativeStream, context), this broke make_queue.

do we need then the device argument in urQueueCreateWithNativeHandle? I think the reason why make_queue passes nullptr for device is that the native queue has been created already in a given device, so it's not like you can create the new queue with the native handle in another device.

@kbenzie , @pbalcer , @rdeodhar : what do you think?

@JackAKirk
Copy link
Contributor Author

@jandres742 @kbenzie @pbalcer @rdeodhar

I've created an issue for the question posed regarding the device argument in the appropriate place: oneapi-src/unified-runtime#778

This PR is a simple bug fix so make_queue will work again. I don't think it is the right place to make any changes to UR interfaces which will delay this fix.
@intel/llvm-gatekeepers can this be merged?

@aelovikov-intel aelovikov-intel merged commit d9deb25 into intel:sycl Aug 4, 2023
steffenlarsen pushed a commit that referenced this pull request Aug 30, 2023
This PR adds missing functions in the hip backend to allow for
interoperability in programs that create sycl objects from native hip
objects. The new function implementations are:

- `make_device`
- `make_queue`
- `make_event`

Note that it would really make sense for
#10491 to be merged first because this
PR makes the same code change in pi2ur, for a fix that is attributed to
#10491.

---------

Signed-off-by: Jack Kirk <[email protected]>
fabiomestre pushed a commit to fabiomestre/llvm that referenced this pull request Sep 26, 2023
Recently in the switch to UR `urQueueCreateFromNativeHandle` changed the
previous behaviour whereby a queue was created with a device taken as
the default device from the context. It changed it so that the queue was
created with the device argument instead. Since the sycl runtime always
passes a nullptr for the device when programmers call
`make_queue(nativeStream, context)`, this broke `make_queue`. This patch
reverts to the previous behaviour before the switch from pi cuda to ur
cuda.

Note that this should also fix `make_queue` for l0 which I also guess
was broken due to the asserts meaning that this line was never reached:
https://github.com/intel/llvm/blob/sycl/sycl/plugins/unified_runtime/ur/adapters/level_zero/queue.cpp#L574.
But I have not tested this.

---------

Signed-off-by: JackAKirk <[email protected]>
fabiomestre pushed a commit to fabiomestre/llvm that referenced this pull request Sep 26, 2023
)

This PR adds missing functions in the hip backend to allow for
interoperability in programs that create sycl objects from native hip
objects. The new function implementations are:

- `make_device`
- `make_queue`
- `make_event`

Note that it would really make sense for
intel#10491 to be merged first because this
PR makes the same code change in pi2ur, for a fix that is attributed to
intel#10491.

---------

Signed-off-by: Jack Kirk <[email protected]>
fabiomestre pushed a commit to fabiomestre/unified-runtime that referenced this pull request Sep 26, 2023
This PR adds missing functions in the hip backend to allow for
interoperability in programs that create sycl objects from native hip
objects. The new function implementations are:

- `make_device`
- `make_queue`
- `make_event`

Note that it would really make sense for
intel/llvm#10491 to be merged first because this
PR makes the same code change in pi2ur, for a fix that is attributed to
#10491.

---------

Signed-off-by: Jack Kirk <[email protected]>
fabiomestre pushed a commit to oneapi-src/unified-runtime that referenced this pull request Sep 27, 2023
This PR adds missing functions in the hip backend to allow for
interoperability in programs that create sycl objects from native hip
objects. The new function implementations are:

- `make_device`
- `make_queue`
- `make_event`

Note that it would really make sense for
intel/llvm#10491 to be merged first because this
PR makes the same code change in pi2ur, for a fix that is attributed to
#10491.

---------

Signed-off-by: Jack Kirk <[email protected]>
veselypeta pushed a commit to veselypeta/llvm that referenced this pull request Sep 28, 2023
)

This PR adds missing functions in the hip backend to allow for
interoperability in programs that create sycl objects from native hip
objects. The new function implementations are:

- `make_device`
- `make_queue`
- `make_event`

Note that it would really make sense for
intel#10491 to be merged first because this
PR makes the same code change in pi2ur, for a fix that is attributed to
intel#10491.

---------

Signed-off-by: Jack Kirk <[email protected]>
szadam pushed a commit to szadam/unified-runtime that referenced this pull request Oct 13, 2023
This PR adds missing functions in the hip backend to allow for
interoperability in programs that create sycl objects from native hip
objects. The new function implementations are:

- `make_device`
- `make_queue`
- `make_event`

Note that it would really make sense for
intel/llvm#10491 to be merged first because this
PR makes the same code change in pi2ur, for a fix that is attributed to
#10491.

---------

Signed-off-by: Jack Kirk <[email protected]>
mdtoguchi pushed a commit to mdtoguchi/llvm that referenced this pull request Oct 18, 2023
Recently in the switch to UR `urQueueCreateFromNativeHandle` changed the
previous behaviour whereby a queue was created with a device taken as
the default device from the context. It changed it so that the queue was
created with the device argument instead. Since the sycl runtime always
passes a nullptr for the device when programmers call
`make_queue(nativeStream, context)`, this broke `make_queue`. This patch
reverts to the previous behaviour before the switch from pi cuda to ur
cuda.

Note that this should also fix `make_queue` for l0 which I also guess
was broken due to the asserts meaning that this line was never reached:
https://github.com/intel/llvm/blob/sycl/sycl/plugins/unified_runtime/ur/adapters/level_zero/queue.cpp#L574.
But I have not tested this.

---------

Signed-off-by: JackAKirk <[email protected]>
omarahmed1111 pushed a commit to omarahmed1111/unified-runtime that referenced this pull request Oct 23, 2023
This PR adds missing functions in the hip backend to allow for
interoperability in programs that create sycl objects from native hip
objects. The new function implementations are:

- `make_device`
- `make_queue`
- `make_event`

Note that it would really make sense for
intel/llvm#10491 to be merged first because this
PR makes the same code change in pi2ur, for a fix that is attributed to
#10491.

---------

Signed-off-by: Jack Kirk <[email protected]>
omarahmed1111 pushed a commit to omarahmed1111/unified-runtime that referenced this pull request Oct 23, 2023
This PR adds missing functions in the hip backend to allow for
interoperability in programs that create sycl objects from native hip
objects. The new function implementations are:

- `make_device`
- `make_queue`
- `make_event`

Note that it would really make sense for
intel/llvm#10491 to be merged first because this
PR makes the same code change in pi2ur, for a fix that is attributed to
#10491.

---------

Signed-off-by: Jack Kirk <[email protected]>
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.

4 participants