-
Notifications
You must be signed in to change notification settings - Fork 115
RFC for "Passed Directly" Customization Point #1999
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
Conversation
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
I have a working proof of concept for this in godbolt, but needs some cleanup before sharing. Let me know if that would be useful to provide here, but likely any needed details can and should be provided in the text. |
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
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.
Apart from the name, the approach looks pretty reasonable to me.
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
only offering customization point and trait as public API Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
|
||
* Is there a better, more concise name than `oneapi::dpl::is_passed_directly_in_onedpl_device_policies[_v]` that | ||
properly conveys the meaning to the users? | ||
* Other names proposed: |
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.
oneapi::dpl::is_passed_directly_to_device
?
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 added the two proposed names you raised and a note to the open questions section.
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 also raised the naming question.
I like the one that proposed by @MikeDvorskiy. I also thought about is_passed_directly_for_hetero_policies
if we think about future extensibility of this trait beyond SYCL (hypothetical scenario for now)
just is_passed_directly
also good enough to me
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.
We might also consider one name for a trait itself and a different name for the customization function
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've separated the names of the ADL customization point and the public trait. This allows us to provide both the struct and value version of the trait, which is the norm.
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.
Excepting the names, I am OK with that proposal,
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
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've also checked the proposal with the example, and it looks good to me since it is well defined and implementable.
Let me approve it. Note that I have not participated in the discussions, so I may lack some intricate details and it may be better to get an additional approval.
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 have reviewed the changes made since my approval, and I confirm it.
|
||
* Is there a better, more concise name than `oneapi::dpl::is_passed_directly_in_onedpl_device_policies[_v]` that | ||
properly conveys the meaning to the users? | ||
* Other names proposed: |
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 also raised the naming question.
I like the one that proposed by @MikeDvorskiy. I also thought about is_passed_directly_for_hetero_policies
if we think about future extensibility of this trait beyond SYCL (hypothetical scenario for now)
just is_passed_directly
also good enough to me
specialized and return `std::true_type{}` if the iterator type is "passed directly" and `std::false_type{}` otherwise. | ||
|
||
Additionally, oneDPL will provide a public trait, | ||
`inline constexpr bool oneapi::dpl::is_passed_directly_in_onedpl_device_policies_v<T>`, indicating if the iterator type |
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.
Are we also going to introduce a struct as a trait? Like below
template <typename T>
struct oneapi::dpl::is_passed_directly_in_onedpl_device_policies {};
I would write an honest template variable
`inline constexpr bool oneapi::dpl::is_passed_directly_in_onedpl_device_policies_v<T>`, indicating if the iterator type | |
```cpp | |
template <typename T | |
inline constexpr bool oneapi::dpl::is_passed_directly_in_onedpl_device_policies_v; |
indicating if the iterator type
instead of what we have because it look like partial specialization or like explicit instantiation from the C++ standpoint. Until of course we want to keep some implementation freedom, which I don't see the space for in this case
|
||
* Is there a better, more concise name than `oneapi::dpl::is_passed_directly_in_onedpl_device_policies[_v]` that | ||
properly conveys the meaning to the users? | ||
* Other names proposed: |
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.
We might also consider one name for a trait itself and a different name for the customization function
Signed-off-by: Dan Hoeflinger <[email protected]>
29a8374
The draft implementation #2126 has been updated with the current state of the RFC. I added the body-less ADL customization point function to the tests, and added the struct version of the public trait (and use it in a few places where it allows for more concise code). I don't want to focus on that implementation in this phase, but I wanted to ensure there was nothing problematic about the design, and I don't find anything so far. |
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
I've added some comments about derived classes or non-exact matches for implemented ADL functions, after doing some exploration on my own. So far, I don't see any big dangers but its something worth thinking more about. Generally, ambiguity issues can be resolved by implementing a more specific ADL function for the type in question. It just must be communicated to the user to be aware of this in documentation / specification. |
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Co-authored-by: Ruslan Arutyunyan <[email protected]>
Provides a proposal for a public customization point for users to define to indicate if their types are passed directly to sycl kernels.
This RFC is intended to address and resolve #1939.
Here is a working proof of concept to play with: https://godbolt.org/z/jvo1esoeb (updated)