Repository navigation
Improve SYCL error handling - #58
Conversation
82a3a4f to
66eb5e5
Compare
szymon-zadworny
left a comment
szymon-zadworny
left a comment
There was a problem hiding this comment.
Most of the provided queue constructors cannot fail as per the SYCL 2020 spec. Please revert the mentioned changes and any of their usage sites to the previous state.
There was a problem hiding this comment.
This change moves more logic to the C++ side without additional benefits. Revert this.
There was a problem hiding this comment.
This change moves more logic to the C++ side. Revert this.
| fn new_queue() -> UniquePtr<Queue>; | ||
| fn new_queue_immediate() -> UniquePtr<Queue>; | ||
| fn new_queue_from_device(device: &Device) -> UniquePtr<Queue>; | ||
| fn new_queue() -> Result<UniquePtr<Queue>>; |
There was a problem hiding this comment.
This cannot fail. Quoting the spec:
default_selector_v
Select a SYCL device from any supported SYCL
backend based on an implementation-defined
heuristic. Since all implementations must support
at least one device, this selector must always
return a device.
Revert this.
| fn new_queue_from_device(device: &Device) -> UniquePtr<Queue>; | ||
| fn new_queue() -> Result<UniquePtr<Queue>>; | ||
| fn new_queue_immediate() -> Result<UniquePtr<Queue>>; | ||
| fn new_queue_from_device(device: &Device) -> Result<UniquePtr<Queue>>; |
There was a problem hiding this comment.
This cannot fail - the spec doesn't mention any exceptions for queue(device const &).
Revert this.
| fn new_queue_immediate() -> UniquePtr<Queue>; | ||
| fn new_queue_from_device(device: &Device) -> UniquePtr<Queue>; | ||
| fn new_queue() -> Result<UniquePtr<Queue>>; | ||
| fn new_queue_immediate() -> Result<UniquePtr<Queue>>; |
There was a problem hiding this comment.
This cannot fail. The spec doesn't mention any crashes w.r.t. creating an in-order queue, and the sycl_ext_intel_queue_immediate_command_list extension mentions that the immediate property is just a hint which is ignored on non-matching devices.
Revert this.
| use sycl_rs::prelude::*; | ||
|
|
||
| fn main() { | ||
| fn main() -> sycl_rs::Result<()> { |
| ); | ||
| } | ||
| } | ||
| Ok(()) |
|
|
||
| fn main() -> sycl_rs::Result<()> { | ||
| let mut queue = Queue::new(); | ||
| let mut queue = Queue::new()?; |
| #[tokio::main] | ||
| async fn main() -> sycl_rs::Result<()> { | ||
| let mut queue = Queue::new(); | ||
| let mut queue = Queue::new()?; |
| let copy_count = buf_count / 2; | ||
|
|
||
| let mut queue = Queue::new(); | ||
| let mut queue = Queue::new()?; |
66eb5e5 to
5f6cd78
Compare
|
@szymon-zadworny please re-review |
| impl Queue { | ||
| /// Construct a `Queue` based on the device returned from the default selector. | ||
| /// | ||
| /// A C++ exception during device selection or queue construction terminates the process. | ||
| pub fn new() -> Self { | ||
| Self(ffi::new_queue()) | ||
| } | ||
|
|
||
| /// Construct an immediate `Queue` based on the device returned from the default selector. | ||
| /// | ||
| /// A C++ exception during device selection or queue construction terminates the process. |
There was a problem hiding this comment.
These comments are redundant since these methods shouldn't throw exceptions in the first place as per the standard. If these methods cause a crash this is a SYCL runtime bug which shouldn't happen on conformant SYCL implementations.
5f6cd78 to
adaed09
Compare
|
Improve SYCL error handling