[SYCL] queue use perfect forwarding - #20867
Conversation
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Failure
e8e1aa1 to
0567a41
Compare
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Failure
0567a41 to
d6a5400
Compare
GitHub Actions
Failure
GitHub Actions
Inactive
GitHub Actions
Failure
GitHub Actions
Inactive
d6a5400 to
d8e2a34
Compare
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Failure
GitHub Actions
Inactive
GitHub Actions
Failure
GitHub Actions
Inactive
| detail::code_location::current()) { | ||
| std::enable_if_t<ext::oneapi::experimental::is_property_list< | ||
| std::decay_t<PropertiesT>>::value, | ||
| event> single_task(PropertiesT &&Properties, |
There was a problem hiding this comment.
You are changing a public API. Is it really necessary?
@gmlueck Could you please comment if spec gives us freedom to accept Properties by universal reference?
There was a problem hiding this comment.
In general, we should not change the API of a public function unless we also change the specification. In this specific case (and the cases below), the function is also deprecated. I think it does not make sense to spend time optimizing deprecated APIs because we plan to remove them soon anyway.
There was a problem hiding this comment.
Oh, I miss it is depricated. So in that case, I totally agree with @gmlueck
There was a problem hiding this comment.
ok then, I agree too, changing PropertiesT was by chance of changing nearby code, will be reverted
| void>::value)) { | ||
| return detail::submit_kernel_direct_single_task<KernelName, true>( | ||
| *this, KernelFunc, {}, Properties, TlsCodeLocCapture.query()); | ||
| *this, KernelFunc, {}, std::forward<PropertiesT>(Properties), |
There was a problem hiding this comment.
Why haven't you updated the else path? And what about the subsequent calls down to the stack? If you forward here, but all next calls in the stack still accept by value, then this optimization makes little sense.
| detail::code_location::current()) { | ||
| std::enable_if_t<ext::oneapi::experimental::is_property_list< | ||
| std::decay_t<PropertiesT>>::value, | ||
| event> single_task(event DepEvent, PropertiesT &&Properties, |
There was a problem hiding this comment.
Same question here as above
| const detail::code_location &CodeLoc) { | ||
| detail::tls_code_loc_t TlsCodeLocCapture(CodeLoc); | ||
| return impl->memset(Ptr, Value, Count, {DepEvent}, | ||
| return impl->memset(Ptr, Value, Count, {std::move(DepEvent)}, |
There was a problem hiding this comment.
The right fix here should be to use sycl::span in the queue_impl::memset to accept DepEvents to avoid temporary std::vector creation.
|
This pull request is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be automatically closed in 30 days. |
|
This pull request was closed because it has been stalled for 30 days with no activity. |
3 participants
Use perfect forwarding in queue inline implementations. In the same place use event move if possible. For sake of performance.
As the change is small one. Most benchmarks show no performance improvement, but some have improved, e.g. (see difference between green lines above and two last points of red line marked in a circle)
Not using perfect forwarding in the API taking universal references is a wrong pattern. I want it to be fixed, in order to