Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect profiling-tag timestamp ordering on OpenCL GPUs by changing the Unified Runtime (UR) OpenCL adapter to record timestamps using a lightweight real device command (buffer fill) instead of synchronization-only commands whose profiling timestamps can be misordered on some drivers. This helps ensure profiling data ordering invariants hold and un-masks previously disabled SYCL ProfilingTag E2E coverage for opencl:gpu.
Changes:
- Implement
urEnqueueTimestampRecordingExpfor OpenCL usingclEnqueueFillBufferon a lazily created per-context internal buffer (requires a profiling-enabled queue; otherwise reports unsupported). - Update SYCL scheduler ProfilingTag enqueue to attempt native timestamp recording via
call_nocheckand fall back to a barrier onUR_RESULT_ERROR_UNSUPPORTED_FEATURE; updatesubmit_profiling_tag()to route non-aspect + profiling-enabled queues through the internal profiling-tag CG path. - Split/adjust unit tests for both “timestamp recording works” and “timestamp recording unsupported → barrier fallback”, and re-enable OpenCL GPU E2E tests by removing the UNSUPPORTED lines.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| unified-runtime/source/adapters/opencl/event.cpp | Implements OpenCL timestamp recording via a small buffer fill and requires queue profiling. |
| unified-runtime/source/adapters/opencl/context.hpp | Adds lazy, thread-safe internal buffer storage to support the OpenCL timestamp emulation. |
| sycl/source/detail/scheduler/commands.cpp | Uses non-throwing UR calls for timestamp recording with an explicit unsupported-feature fallback to a barrier. |
| sycl/include/sycl/ext/oneapi/experimental/profiling_tag.hpp | Submits an internal profiling-tag CG when queue profiling is enabled even without the device aspect. |
| sycl/unittests/Extensions/ProfilingTag.cpp | Expands unit tests to cover native timestamp path vs unsupported-feature barrier fallback. |
| sycl/test-e2e/ProfilingTag/profiling_queue.cpp | Re-enables OpenCL GPU E2E coverage by removing the UNSUPPORTED directive. |
| sycl/test-e2e/ProfilingTag/in_order_profiling_queue.cpp | Re-enables OpenCL GPU E2E coverage by removing the UNSUPPORTED directive. |
| std::vector<cl_event> CLWaitEvents(numEventsInWaitList); | ||
| for (uint32_t I = 0; I < numEventsInWaitList; I++) | ||
| CLWaitEvents[I] = cast(phEventWaitList[I])->CLEvent; | ||
|
|
||
| const cl_uint Pattern = 0; | ||
| cl_event Event = nullptr; | ||
| CL_RETURN_ON_FAILURE(clEnqueueFillBuffer( | ||
| Queue->CLQueue, Buffer, &Pattern, sizeof(Pattern), /*offset=*/0, | ||
| /*size=*/sizeof(Pattern), numEventsInWaitList, CLWaitEvents.data(), | ||
| ifUrEvent(phEvent, Event))); |
| for (uint32_t i = 0; i < DeviceCount; i++) { | ||
| ur::opencl::urDeviceRelease(cast(Devices[i])); | ||
| } | ||
| if (TimestampRecordingBuffer) { | ||
| clReleaseMemObject(TimestampRecordingBuffer); | ||
| } | ||
| if (IsNativeHandleOwned) { | ||
| clReleaseContext(CLContext); | ||
| } |
| if (!IsInOrderQueue) | ||
| Adapter.call<UrApiKind::urEventRelease>(PreTimestampMarkerEvent); | ||
| // A barrier with an empty wait list waits for all previously-submitted | ||
| // work and blocks subsequent work, providing the same ordering semantics | ||
| // as a native profiling tag. | ||
| if (auto Result = | ||
| Adapter.call_nocheck<UrApiKind::urEnqueueEventsWaitWithBarrier>( | ||
| MQueue->getHandleRef(), | ||
| /*num_events_in_wait_list=*/0, | ||
| /*event_wait_list=*/nullptr, Event); | ||
| Result != UR_RESULT_SUCCESS) | ||
| return Result; |
There was a problem hiding this comment.
If it's out-of-order queue, then we already have a barrier submitted before pre-timestamp, so we will submit barrier twice here.
Can we assign PreTimestampMarkerEvent to Event instead of releasing it and insert barrier under "else":
if (!IsInOrderQueue) {
// reuse PreTimestampMarkerEvent as output event
} else {
// submit barrier
}
| if (TimestampResult != UR_RESULT_SUCCESS) | ||
| return TimestampResult; |
There was a problem hiding this comment.
We need to release PreTimestampMarkerEvent event under if (TimestampResult != UR_RESULT_SUCCESS) as well if it's out-of-order queue.
Ideally, we should use OwnedUrEvent from ur_utils.hpp so that release is handled automatically via raii.
But it probably requires a little extension of OwnedUrEvent like this:
OwnedUrEvent &operator=(OwnedUrEvent &&Other) {
if (this != &Other) {
if (MEvent.has_value())
MAdapter->call<UrApiKind::urEventRelease>(*MEvent);
MEvent = Other.MEvent;
MAdapter = Other.MAdapter;
Other.MEvent = std::nullopt;
}
return *this;
}
OwnedUrEvent &operator=(const OwnedUrEvent &Other) = delete;
So I don't insist on doing it in this PR.
|
adapters/opencl LGTM but please apply copilot review feedback |
|
@crystarm please rebase and resolve conflicts |
85c9d4b to
2f969b5
Compare
|
@bratpiorka |
|
@crystarm ProfilingTag/in_order_profiling_queue.cpp fails on native cpu |
I'm sorry, I didn't notice that (ᵕ •_•) |
|
@KornevNikita I think it's gonna be all right now. Could you please approve the workflow? |
|
@crystarm still fails |
22ba5e9 to
020e26b
Compare
|
@KornevNikita The previous fix switched I think it should FINALLY be cool now............. 😭😭😭🙏🙏🙏🙏 |
Well, it is not cool. I've no idea what else could be wrong, everything passed locally, including all Native CPU E2E test runs. I'll keep looking into it. |
|
#22810 changed profiling tag timestamp handling so that the runtime no longer initializes The previous Native CPU fallback fix converted the fallback event into a regular profiling event and relied on When I created the original branch for this PR, #22810 was not yet part of Merging the current I wanted to keep the original branch intact, preserve the exact changes already reviewed and agreed upon with the maintainers, and avoid force-pushing anything. So I created The new fix keeps the fallback event marked as a profiling tag and uses its device-recorded completion timestamp for The replacement branch is now available in #23153. If #23153 is accepted, this PR should be closed. |
Related to #22229.
submit_profiling_tagreturned timestamps in the wrong order onopencl:gpu: the end tag'scommand_endcould be earlier than the completion of work submitted before it, soE2End <= EndTagEndfailed and the e2e tests were masked withUNSUPPORTED: opencl && gpu.Why
OpenCL devices don't advertise
ext_oneapi_queue_profiling_tag(the adapter hard-codesUR_DEVICE_INFO_TIMESTAMP_RECORDING_SUPPORT_EXPtofalse), so the tag fell back toext_oneapi_submit_barrier()and read its profiling info straight off the barrier event.The NEO driver timestamps
clEnqueueBarrierWithWaitList/clEnqueueMarkerWithWaitListwhen the command enters the pipeline, not when the preceding work completes. So the barrier'sCL_PROFILING_COMMAND_ENDcan be earlier than the end of a kernel enqueued before it.I confirmed this with a small pure-OpenCL reproducer on Intel Iris Xe (NEO 26.05.037020): any synchronization-only tag (barrier/marker, empty or explicit wait list) fails 100% of the time, while any real device command (kernel or buffer fill) gives correct timestamps - including a fill into a
dedicated buffer, which is what this PR does.
Changes
unified-runtime/.../opencl/event.cpp: implement urEnqueueTimestampRecordingExpusingclEnqueueFillBufferon a small internal buffer, whose timestamps do reflect completion of prior work. It needs command profiling, so if the queue isn't profiling-enabled we returnUR_RESULT_ERROR_UNSUPPORTED_FEATURE`.unified-runtime/.../opencl/context.hpp: lazily-created, thread-safe 4-byte buffer per context, released with the context. It's never read - only the fill command's timestamps are used.sycl/.../scheduler/commands.cpp(CGType::ProfilingTag): issue the recording withcall_nocheckand fall back to a barrier onUR_RESULT_ERROR_UNSUPPORTED_FEATURE. Also addresses the existingTODOhere.sycl/.../experimental/profiling_tag.hpp: when the device lacks the aspect but the queue has profiling enabled, submit a profiling-tag command group (reusing the scheduler's in-order/out-of-order marker+barrier handling) instead of a bare barrier.The device aspect stays
falsefor OpenCL: the emulation needs queue profiling, but the aspect promises the tag works without it. Enabling the aspect (e.g. via an internal profiling queue) is a possible follow-up.Level Zero / CUDA / HIP are unaffected - they advertise the aspect and already implement the recording, so they never take the fallback branch. The scheduler change only adds the fallback branch and turns a throwing call into a checked one; the success path is unchanged.
Testing
Unit tests (
sycl/unittests/Extensions/ProfilingTag.cpp): split the old fallback test into two - native recording used on a profiling queue without the aspect, and the barrier fallback when the backend reports the recording unsupported.E2E: dropped
UNSUPPORTED: opencl && gpufromProfilingTag/in_order_profiling_queue.cppandProfilingTag profiling_queue.cpp.The reproducer's dedicated-buffer fill (matching this implementation) passes 200/200 iterations on Intel Iris Xe, vs. the old barrier fallback failing 200/200.
The NEO timestamping behavior is arguably a driver bug worth reporting separately; this is a runtime workaround that doesn't touch the public API or other backends.
P.S. I may have gone a little overboard with the code comments. If anyone else feels the same way, I will clean them up.