[SYCL][L0] Force submit of dependent open command batches #6199
Conversation
Signed-off-by: Sergey V Maslov <[email protected]>
Signed-off-by: Sergey V Maslov <[email protected]>
| if (Queue == CurQueue && | ||
| OpenCommandList->second.isCopy(Queue) == UseCopyEngine) { | ||
| // Don't force execute the batch yet since the new command | ||
| // is going to the same open batch as the dependent event. | ||
| } else { | ||
| if (auto Res = Queue->executeOpenCommandList( | ||
| OpenCommandList->second.isCopy(Queue))) | ||
| return Res; | ||
| } |
There was a problem hiding this comment.
From this code it looks like we always force execute open command list if it is compute command list. Could you please explain why behavior needs to be different for copy and compute command lists? I think comment https://github.com/intel/llvm/pull/6199/files#diff-15dd1eb076d2164bd9e87d9057f05f652a716498e8cdf5975e564c65309a0985R1706-R1709 should mention about this difference.
There was a problem hiding this comment.
It is not different for copy vs. compute. This code force-submits copy if the dependent command is compute, and vice versa. Perhaps, you were confused by this check OpenCommandList->second.isCopy(Queue) == UseCopyEngine looking like it is about copy-only, but I think it is not :)
There was a problem hiding this comment.
Oh, I see, yes, I was confused by this check.
| // The caller of createAndRetainPiZeEventList must already hold | ||
| // a lock of the CurQueue. Additionally lock the Queue if it | ||
| // is different from CurQueue. | ||
| auto Lock = ((Queue == CurQueue) ? std::unique_lock<pi_shared_mutex>() | ||
| : std::unique_lock(Queue->Mutex)); |
There was a problem hiding this comment.
Problem is that this can cause a deadlock itself if we have one thread where the caller locks Queue1 and the callee locks Queue2 and another thread where the caller locks Queue2 and the callee locks Queue1.
I understand that this bug existed before your patch, so I am ok this to be fixed separately. I believe we will need to move createAndRetainPiZeEventList out of queue lock.
There was a problem hiding this comment.
I agree. Added TODO comment to rework this further
Signed-off-by: Sergey V Maslov <[email protected]>
| if (Queue == CurQueue && | ||
| OpenCommandList->second.isCopy(Queue) == UseCopyEngine) { | ||
| // Don't force execute the batch yet since the new command | ||
| // is going to the same open batch as the dependent event. | ||
| } else { | ||
| if (auto Res = Queue->executeOpenCommandList( | ||
| OpenCommandList->second.isCopy(Queue))) | ||
| return Res; | ||
| } |
There was a problem hiding this comment.
Oh, I see, yes, I was confused by this check.
Previously we were only force-submitting batches seen as dependency in different PI queue.
Now, we also properly submit copy/compute open command-lists within the same PI queue too.
E2E test: https://github.com/alexanderfle/llvm-test-suite/blob/reuse_events_in_L0_tests/SYCL/Plugin/level_zero_inorder_interleaving_kernel_copy.cpp
Signed-off-by: Sergey V Maslov [email protected]