[NFCI][SYCL] Refactor HandlerAccess::postProcess
#19203
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Implements the idea from the earlier TODO comment. Instead of having a hacky `handler::MLastEvent` just `swap` original and post-processing `handler`, so that "natural" `finalize` will work on the latest task in the chain.
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
aelovikov-intel
commented
Jun 27, 2025
| // Reductions implementation need access to private members of handler. Those | ||
| // are limited to those below. | ||
| inline void finalizeHandler(handler &CGH); | ||
| template <class FunctorTy> void withAuxHandler(handler &CGH, FunctorTy Func); |
There was a problem hiding this comment.
withAuxHandler declaration should have been removed earlier when postProcess was introduced instead of it.
| @@ -488,6 +488,9 @@ class __SYCL_EXPORT handler { | |||
| /// \param Graph is a SYCL command_graph | |||
| handler(std::shared_ptr<ext::oneapi::experimental::detail::graph_impl> Graph); | |||
| #endif | |||
| handler(std::unique_ptr<detail::handler_impl> &&HandlerImpl); | |||
|
|
|||
| ~handler(); | |||
There was a problem hiding this comment.
std::unique_ptr<T> dtor needs a complete T, so move the definition to the handler.cpp.
| @@ -577,11 +571,10 @@ event handler::finalize() { | |||
| DiscardEvent = !KernelUsesAssert; | |||
| } | |||
|
|
|||
| #ifdef __INTEL_PREVIEW_BREAKING_CHANGES | |||
| if (!DiscardEvent) { | |||
| LastEventImpl = detail::event_impl::create_completed_host_event(); | |||
There was a problem hiding this comment.
No idea why we were using create_completed_host_event before. @slawekptak , any thoughts?
| @@ -635,29 +627,32 @@ event handler::finalize() { | |||
|
|
|||
| if (DiscardEvent) { | |||
| EnqueueKernel(); | |||
| #ifndef __INTEL_PREVIEW_BREAKING_CHANGES | |||
| LastEventImpl->setStateDiscarded(); | |||
There was a problem hiding this comment.
Line 654 on the right does create_discarded_event instead, which, IMO, is even more readable.
There was a problem hiding this comment.
I agree and it's also going away, so no real concerns either way.
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
steffenlarsen
approved these changes
Jun 30, 2025
| @@ -635,29 +627,32 @@ event handler::finalize() { | |||
|
|
|||
| if (DiscardEvent) { | |||
| EnqueueKernel(); | |||
| #ifndef __INTEL_PREVIEW_BREAKING_CHANGES | |||
| LastEventImpl->setStateDiscarded(); | |||
There was a problem hiding this comment.
I agree and it's also going away, so no real concerns either way.
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
are failing in other PRs too. XFAILs seem to be handled in #19215. FAILs look similar to earlier reported #18576 (Linux), I suspect we just update our Win drivers to the same state. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements the idea from the earlier TODO comment. Instead of having a hacky
handler::MLastEventjustswaporiginal and post-processinghandler, so that "natural"finalizewill work on the latest task in the chain.