[SYCL] Refactor queue to improve ABI stability #985
Conversation
alexbatashev
requested review from
jbrodman,
AlexeySachkov,
romanovvlad and
garimagu
alexbatashev
force-pushed
the
private/abatashe/refactor_queue
branch
from
b4d1b76 to
700ec1c
Compare
| const size_t MaxNumQueues = 256; | ||
|
|
||
| enum QueueOrder { Ordered, OOO }; | ||
|
|
||
| class queue_impl { | ||
| public: | ||
| /// Constructs a SYCL queue with an async_handler and property_list provided | ||
| /// from a device. |
There was a problem hiding this comment.
I'm not sure it's phrased carefully. "async_handler and property_list provided from a device" - sounds like error handler and property list are associated with "a device" rather than "a queue".
| event submit_impl(T cgf, std::shared_ptr<queue_impl> self) { | ||
| handler Handler(std::move(self), m_HostQueue); | ||
| cgf(Handler); | ||
| /// Performs actual command group submission to the queue. |
There was a problem hiding this comment.
Do we need add "actual" here? I'm not sure that context is clear for everyone.
alexbatashev
force-pushed
the
private/abatashe/refactor_queue
branch
from
700ec1c to
9692303
Compare
| template <class Obj> | ||
| friend decltype(Obj::impl) detail::getSyclObjImpl(const Obj &SyclObject); | ||
|
|
||
| /// A template-free version of submit. | ||
| event submit_impl(function_class<void(handler &)> CGH); |
There was a problem hiding this comment.
Are we not removing T in submit because it's in the spec?
What failure will we see in runtime if T passed to submit is not function_class?
There was a problem hiding this comment.
@garimagu table 4.22 of the spec explicitly specifies what T may be. Those are all callable objects of the same signature. So, user is not expected to pass anything incompatible with function_class and they get a compilation failure if the do. Furthermore, there're no behavior changes, even without this patch custom object causes failure.
alexbatashev
force-pushed
the
private/abatashe/refactor_queue
branch
from
9692303 to
c415a98
Compare
Signed-off-by: Alexander Batashev <[email protected]>
Signed-off-by: Alexander Batashev <[email protected]>
Signed-off-by: Alexander Batashev <[email protected]>
Signed-off-by: Alexander Batashev <[email protected]>
Signed-off-by: Alexander Batashev <[email protected]>
Signed-off-by: Alexander Batashev <[email protected]>
Signed-off-by: Alexander Batashev <[email protected]>
Signed-off-by: Alexander Batashev <[email protected]>
Signed-off-by: Alexander Batashev <[email protected]>
Signed-off-by: Alexander Batashev <[email protected]>
Signed-off-by: Alexander Batashev <[email protected]>
Signed-off-by: Alexander Batashev <[email protected]>
Signed-off-by: Alexander Batashev <[email protected]>
Signed-off-by: Alexander Batashev <[email protected]>
Signed-off-by: Alexander Batashev <[email protected]>
Signed-off-by: Alexander Batashev <[email protected]>
Signed-off-by: Alexander Batashev <[email protected]>
alexbatashev
force-pushed
the
private/abatashe/refactor_queue
branch
from
c415a98 to
7e1a82f
Compare
| /// Constructs a SYCL queue with an async_handler and property_list provided | ||
| /// form a device and a context. | ||
| /// | ||
| /// @param Device is a pointer to device_impl. |
There was a problem hiding this comment.
Do you see any value in the comments like this?
This comment repeats information, which I can get form the constructor declaration - I see the type of Device parameter.
You could say something useful about relations between the queue and device - e.g. that commands submitted to the queue will be executed on the Device.
I would expect a comment clarifying why Device is needed to construct the queue, not what is the parameter type.
I really appreciate your work with commenting the code, but please, do not add comments just for the sake of having some comments.
Signed-off-by: Alexander Batashev <[email protected]>
This patch is a part of effort to decouple SYCL Runtime library
interface from its actual implementation. The goal is to improve
SYCL ABI/API compatibility between different versions of library.
The following modifications were applied to queue and queue_impl
classes:
it with forward declaration.
function_class<void(handler&)>Applying aforementioned changes requires modifying other header files.
While some of those may seem as a regression, affected classes will be
covered by future patches.
Signed-off-by: Alexander Batashev [email protected]