-
Notifications
You must be signed in to change notification settings - Fork 731
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SYCL] Fix EnableIfOutputIteratorT trait #12834
Conversation
sycl/include/sycl/buffer.hpp
Outdated
static_assert(!std::is_const_v<std::remove_reference_t<decltype(*FinalData)>>, | ||
"set_final_data must be called with a non-const iterator!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this and not the fix in EnableIfOutputIteratorT
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reasoning was that by adding it to EnableIfOutputIteratorT
, when we call set_final_data_internal with a const iterator it simply gets ignored by overload resolution and the compiler diagnostics will most likely be confusing for someone who is not that familiar with template errors. Furthermore, this is an easy mistake to make when writing SYCL code and therefore it would be nice to get a clear error message telling you what you did wrong and thats what a static_assert does. If you're not convinced however, I can add it to EnableIfOutputIteratorT
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it simply gets ignored by overload resolution
And that's what the spec says should happen, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide the part of the spec that says this?
All I can see is that the spec says what to do if the iterator is not const, I cant see anything regarding what to do if it is const.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SYCL:
Destination can be either an output iterator or a std::weak_ptr.
C++:
The output_iterator concept is a refinement of input_or_output_iterator, adding the requirement that it can be used to write values of type and value category encoded by T (via indirectly_writable). equality_comparable is not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks for the clarification!
I will make the suggested changes!
@aelovikov-intel ping |
sycl/include/sycl/detail/common.hpp
Outdated
@@ -35,7 +35,9 @@ using EnableIfOutputPointerT = std::enable_if_t< | |||
|
|||
template <typename DataT> | |||
using EnableIfOutputIteratorT = std::enable_if_t< | |||
/*is_output_iterator<DataT>::value &&*/ !std::is_pointer_v<DataT>>; | |||
/*is_output_iterator<DataT>::value &&*/ !std::is_pointer_v<DataT> && | |||
!std::is_const_v<std::remove_reference_t< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Line 34 has the same bug.
- Have you checked if comment at line 30 is up-to-date?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check if its prossible to uncomment the is_output_iterator trait then I'll get back to you. If this is unblocked then this PR is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aelovikov-intel It looks like the problem was with void* ptr so I added a full specialization for that case. This enabled me to uncomment the is_output_iterator. Let me know of any concerns!
There is some confusion going on. The test test-e2e/Basic/buffer/buffer.cpp is calling |
My opinion here is that the test was wrong, but we didn't catch it because the specification was ambiguous. If we clarify the specification, we can update the test. But I'd like to hear from @gmlueck. |
I agree. The type Note that there are other potential breaking changes being discussed in KhronosGroup/SYCL-CTS#877, so it might make sense to wait for that to resolve and make all the changes at once. |
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. |
This PR fixes the implementation of the
EnableIfOutputIteratorT
type trait that is used in the overloads of the set_final_data_internal method of the buffer class. This makes it so that set_final_data will be disabled if its argument is a non-modifiable iterator/pointer.