-
Notifications
You must be signed in to change notification settings - Fork 798
[SYCL][NFC] Cache the sycl::device instance in queue_impl #20764
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
base: sycl
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,10 +100,10 @@ class queue_impl : public std::enable_shared_from_this<queue_impl> { | |
| /// to the queue. | ||
| /// \param AsyncHandler is a SYCL asynchronous exception handler. | ||
| /// \param PropList is a list of properties to use for queue construction. | ||
| queue_impl(device_impl &Device, const async_handler &AsyncHandler, | ||
| queue_impl(const device &Device, const async_handler &AsyncHandler, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels quite suspicious to have an implementation-layer object take as a constructor argument a public API object. Can we not, for example, directly store the |
||
| const property_list &PropList, private_tag tag) | ||
| : queue_impl(Device, getDefaultOrNew(Device), AsyncHandler, PropList, | ||
| tag) {}; | ||
| : queue_impl(Device, getDefaultOrNew(*getSyclObjImpl(Device)), | ||
| AsyncHandler, PropList, tag) {}; | ||
|
|
||
| /// Constructs a SYCL queue with an async_handler and property_list provided | ||
| /// form a device and a context. | ||
|
|
@@ -114,7 +114,7 @@ class queue_impl : public std::enable_shared_from_this<queue_impl> { | |
| /// constructed. | ||
| /// \param AsyncHandler is a SYCL asynchronous exception handler. | ||
| /// \param PropList is a list of properties to use for queue construction. | ||
| queue_impl(device_impl &Device, std::shared_ptr<context_impl> &&Context, | ||
| queue_impl(const device &Device, std::shared_ptr<context_impl> &&Context, | ||
| const async_handler &AsyncHandler, const property_list &PropList, | ||
| private_tag) | ||
| : MDevice(Device), MContext(std::move(Context)), | ||
|
|
@@ -143,7 +143,7 @@ class queue_impl : public std::enable_shared_from_this<queue_impl> { | |
| "Queue compute index must be a non-negative number less than " | ||
| "device's number of available compute queue indices."); | ||
| } | ||
| if (!MContext->isDeviceValid(Device)) { | ||
| if (!MContext->isDeviceValid(*getSyclObjImpl(Device))) { | ||
| if (MContext->getBackend() == backend::opencl) | ||
| throw sycl::exception( | ||
| make_error_code(errc::invalid), | ||
|
|
@@ -174,7 +174,7 @@ class queue_impl : public std::enable_shared_from_this<queue_impl> { | |
| trySwitchingToNoEventsMode(); | ||
| } | ||
|
|
||
| queue_impl(device_impl &Device, context_impl &Context, | ||
| queue_impl(const device &Device, context_impl &Context, | ||
| const async_handler &AsyncHandler, const property_list &PropList, | ||
| private_tag Tag) | ||
| : queue_impl(Device, Context.shared_from_this(), AsyncHandler, PropList, | ||
|
|
@@ -192,7 +192,7 @@ class queue_impl : public std::enable_shared_from_this<queue_impl> { | |
| queue_impl(ur_queue_handle_t UrQueue, context_impl &Context, | ||
| const async_handler &AsyncHandler, const property_list &PropList, | ||
| private_tag) | ||
| : MDevice([&]() -> device_impl & { | ||
| : MDevice([&]() -> device { | ||
| ur_device_handle_t DeviceUr{}; | ||
| adapter_impl &Adapter = Context.getAdapter(); | ||
| // TODO catch an exception and put it to list of asynchronous | ||
|
|
@@ -206,7 +206,7 @@ class queue_impl : public std::enable_shared_from_this<queue_impl> { | |
| make_error_code(errc::invalid), | ||
| "Device provided by native Queue not found in Context."); | ||
| } | ||
| return *Device; | ||
| return createSyclObjFromImpl<device>(*Device); | ||
| }()), | ||
| MContext(Context.shared_from_this()), MAsyncHandler(AsyncHandler), | ||
| MPropList(PropList), MQueue(UrQueue), | ||
|
|
@@ -297,10 +297,13 @@ class queue_impl : public std::enable_shared_from_this<queue_impl> { | |
|
|
||
| std::weak_ptr<context_impl> getContextImplWeakPtr() const { return MContext; } | ||
|
|
||
| device_impl &getDeviceImpl() const { return MDevice; } | ||
| device_impl &getDeviceImpl() { return *getSyclObjImpl(MDevice); } | ||
|
|
||
| /// \return an associated SYCL device by reference. | ||
| device &get_device() { return MDevice; } | ||
|
|
||
| /// \return an associated SYCL device. | ||
| device get_device() const { return createSyclObjFromImpl<device>(MDevice); } | ||
| device get_device() const { return MDevice; } | ||
|
|
||
| /// \return true if this queue allows for discarded events. | ||
| bool supportsDiscardingPiEvents() const { return MIsInorder; } | ||
|
|
@@ -499,7 +502,7 @@ class queue_impl : public std::enable_shared_from_this<queue_impl> { | |
| ur_queue_handle_t createQueue(QueueOrder Order) { | ||
| ur_queue_handle_t Queue{}; | ||
| ur_context_handle_t Context = MContext->getHandleRef(); | ||
| ur_device_handle_t Device = MDevice.getHandleRef(); | ||
| ur_device_handle_t Device = getSyclObjImpl(MDevice).get()->getHandleRef(); | ||
| /* | ||
| sycl::detail::pi::PiQueueProperties Properties[] = { | ||
| PI_QUEUE_FLAGS, createPiQueueProperties(MPropList, Order), 0, 0, 0}; | ||
|
|
@@ -984,7 +987,7 @@ class queue_impl : public std::enable_shared_from_this<queue_impl> { | |
| /// Protects all the fields that can be changed by class' methods. | ||
| mutable std::mutex MMutex; | ||
|
|
||
| device_impl &MDevice; | ||
| device MDevice; | ||
| const std::shared_ptr<context_impl> MContext; | ||
|
|
||
| /// These events are tracked, but not owned, by the queue. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -220,8 +220,9 @@ Scheduler::GraphBuilder::getOrInsertMemObjRecord(queue_impl *Queue, | |
| // Since all the Scheduler commands require queue but we have only context | ||
| // here, we need to create a dummy queue bound to the context and one of the | ||
| // devices from the context. | ||
| std::shared_ptr<queue_impl> InteropQueuePtr = queue_impl::create( | ||
| Dev, *InteropCtxPtr, async_handler{}, property_list{}); | ||
| std::shared_ptr<queue_impl> InteropQueuePtr = | ||
| queue_impl::create(createSyclObjFromImpl<device>(Dev), *InteropCtxPtr, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely use-after free here, temporary device will die before the next statement and queue will have a stale reference. I'm also surprised it's compilable. How do you bind an rvalue device object to an lvalue reference argument?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I guess this compiles because the queue_impl constructor takes a const reference. It then makes a copy of the object, so it should work, but probably the constructor should take it by value - that should be clean, right? |
||
| async_handler{}, property_list{}); | ||
|
|
||
| MemObject->MRecord.reset(new MemObjRecord{InteropCtxPtr, LeafLimit, | ||
| std::move(AllocateDependency)}); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a guarantee that graph will outlive queue?