[SYCL][PI][L0] Add batching of multiple command into a command list before executing that command list. #2605
Conversation
Pull to update my local fork
list prior to executing that command list.
kbsmith-intel
requested review from
kbobrovs,
pvchupin and
smaslov-intel
as code owners
| } | ||
|
|
||
| pi_result _pi_queue::executeOpenCommandList() { | ||
| if (this->RefCount > 0) { |
There was a problem hiding this comment.
With the queue being locked for the duration of each PI API using this queue, is it really possible for this condition to ever be false?
There was a problem hiding this comment.
Possibly, I am not sure. But if it is always true it is harmless, and we could remove it in a separate change-set.
There was a problem hiding this comment.
I'd rather not add dead code from the beginning. It also gives bad example to others who may be adding more such checks, which certainly don't add clarity to the code.
There was a problem hiding this comment.
OK, will remove this.
| : ZeCommandQueue{Queue}, Context{Context}, Device{Device} {} | ||
| pi_device Device, pi_uint32 QueueBatchSize) | ||
| : ZeCommandQueue{Queue}, Context{Context}, Device{Device}, | ||
| QueueBatchSize{QueueBatchSize} {} |
There was a problem hiding this comment.
why do we need batch size to be a member of a queue instead of just using ZeCommandListBatchSize?
There was a problem hiding this comment.
I was setting this code up for the future. By making this a field of the queue that gets set at queue creation time based on the global ZeCommandListBatchSize global variable/env var, I can allow a heuristic to change this on a queue specific basis without introducing any race/thread unsafe global var uses since fields of the queue are now all guarded in a thread safe manner. So, while not specifically necessary, I think it is an excellent way to make it easier to add heuristics for batching in the future.
There was a problem hiding this comment.
OK, but please clarify this intention of QueueBatchSize in a comment.
| // go ahead and execute what is already in the batched list, | ||
| // and then go on to process this. On exit from executeOpenCommandList | ||
| // ZeOpenCommandList will be nullptr. | ||
| Queue->executeOpenCommandList(); |
There was a problem hiding this comment.
here and in all other places you call executeOpenCommandList please check return code and return if it is not PI_SUCCESS
There was a problem hiding this comment.
OK, I will do that.
…nt in executeOpenCommandList as requested in code review.
The affected source files were relying on transitive includes that were no longer included after recent llvm-project changes (such as 36c6632 ("[IR] Don't include PassInstrumentation.h in PassManager.h (NFC) (#96219)", 2024-06-21)). Original commit: KhronosGroup/SPIRV-LLVM-Translator@97069a601d2e733
[DeviceASAN] Bugfix for GetDeviceType
[DeviceASAN] Bugfix for GetDeviceType
No description provided.