[SYCL][UR][L0 v2] Move all enqueue-related logic to command_list_manager #18837
Conversation
Almost all in_order_queue functions are implemented by just forwarding the call to the command_list_manager. This is needed to implement out-of-order queue without duplicating the implementation.
all command list manager functions now expect ur_event_handle_t as signal event, not ur_event_handle_t*
| ur_result_t ur_command_list_manager::enqueueKernelLaunch( | ||
| ur_kernel_handle_t hKernel, uint32_t workDim, | ||
| const size_t *pGlobalWorkOffset, const size_t *pGlobalWorkSize, | ||
| const size_t *pLocalWorkSize, uint32_t numPropsInLaunchPropList, |
There was a problem hiding this comment.
This behavior that adds multiple kernels to the list, depending on launchPropList seems quite counterintuitive. Currently, the numPropsInLaunchPropList and launchPropList seems like any other argument of the enqueueKernelLaunch that would modify the added kernel, but wouldn't add multiple kernels at once.
Wouldn't it make more sense to either add a separate command_list_manager method that would allow adding multiple kernels, or create a separate structure that would describe the launch parameters, to have a clearer distinction between these two?
There was a problem hiding this comment.
I don't think multiple kernels are added - we only iterate over properties to see if there is cooperative property and if so, we need to call different L0 function to enqueue the kernel.
I agree that this part might use a refactor - at least we could extract the logic for checking the properties to a separate function - let's do that in a separate PR.
| const ur_event_handle_t *phEventWaitList, | ||
| ur_event_handle_t *phEvent); | ||
| /************ Helper methods *************/ | ||
| ur_result_t enqueueGenericCommandListsExp( |
There was a problem hiding this comment.
Shouldn't this be private?
| template <typename Base> locked<Base> lock() { | ||
| std::unique_lock lock{mut_}; | ||
| return locked<Base>(&object_, std::move(lock)); | ||
| } |
There was a problem hiding this comment.
If we introduce more generic approach, wouldn't it work to express the default one using the new one, to not duplicate code?
There was a problem hiding this comment.
Actually, this was a leftover from a previous implementation, this is not needed anymore.
unified-runtime/source/adapters/level_zero/v2/command_list_manager.cpp
Outdated
Show resolved
Hide resolved
| ze_event_handle_t getSignalEvent(ur_event_handle_t hUserEvent, | ||
| ur_command_t commandType); | ||
|
|
||
| /************ Generic queue methods *************/ |
There was a problem hiding this comment.
This is a nitpick, so feel free to ignore, but I preferred the previous appendFoo naming to enqueueFoo. As the methods append to a command-list object rather than submit to a queue object. I understand that a immediate command-list object is effectively a queue, but the command-list might not always be immediate (as in the case where it's being called by the command-buffer API)
There was a problem hiding this comment.
Yeah, I agree. I prefer append naming as well.
There was a problem hiding this comment.
Ok, changed back to append.
Almost all in_order_queue functions are implemented by
just forwarding the call to the command_list_manager.
This is needed to implement out-of-order queue without
duplicating the implementation.