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.
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
RFC for "Passed Directly" Customization Point #1999
base: main
Are you sure you want to change the base?
RFC for "Passed Directly" Customization Point #1999
Changes from 21 commits
45248dd
5ff56d9
643a2cb
6c0fc33
3bacd14
f24f8d2
1145efa
d1d0718
81ad0a2
36b8ec5
b0dac4f
4823c6e
b79e7f5
47b8be3
92b1d33
5c45e7d
8bcc334
27dc9ba
60cd1a9
2ddaf9d
497b8c6
7837319
79adf34
5d704ab
91b77a2
0474d92
29a8374
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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_in_onedpl_device_policies_v
has double info about "oneDPL":I mean "oneapi::dpl" and substring "onedpl".
So, I would suggest name
oneapi::dpl::is_passed_directly_v
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.
For now, I think the idea is to merge this RFC with the naming and location as an open question, to be decided in the discussion of the specification PR. All the same, thanks for your input here.
Also some context for the individual points:
The idea behind this is that the public trait follows the naming convention of the ADL customization point function. The ADL customization point function needs to identify itself as
onedpl
because it user's customizations will be in their own namespaces. However, we could decide not to have the public trait share naming with the ADL function, and I would agree with removing this redundancy then.There was an earlier discussion in this PR RFC for "Passed Directly" Customization Point #1999 (comment) , I think its a difficult decision with many opinions. I don't really have strong opinions here and am happy to go with the group's decision.
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
I would write an honest template variable
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.
Its a good question, and probably needs to be handled in connection with the question of naming, and if the ADL function should be named the same as the trait.
Now I am thinking that it would be better to make their names distinct, and then to offer both the struct and value version of the trait
oneapi::dpl::is_passed_directly_in_onedpl_device_policies[_v]
as you describe. This is the more normal convention, but requires divorcing the names between the ADL customization point and the trait.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.
by
decltype
-ing (see below) function I mean that this customization might look likewithout any body and it would work.
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 would consider this not a typical example. Typically there should be some logic, like in the next example.
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.
Thinking about the way we use it for our own internal iterator types, I think normally there will be some logic, but there are also some iterator types which have signatures like this. (
counting_iterator
,discard_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.
Users can? or users shall? I feel it's a requirement to use at least
constexpr
function for us to be able to get the result at compile time.Also are we going to call it or are we going to "
decltype
" it? I might affect whether it shall beconstexpr
or notThere 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.
"Users can use any logic" it says.
It is a requirement that the function is
constexpr
, and it is stated earlier. Remember that this is not a formal specification but design description.The proposal says "When using device policies, oneDPL will run compile-time checks on argument iterator types by calling is_passed_directly_in_onedpl_device_policies.". It might be not 100% accurate but the intent is clear (at least 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.
I've improved the language a bit to make it clear that any logic may be used, as long as it is
constexpr
logic.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.
this function may be rewritten simpler. See below:
Furthermore, it be be rewritten without a body if we want to support
decltype
customization styleIt seems like nobody seriously considered functions without body design aspect
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 is more concise but not really simpler. Brevity does not equal to clarity. I personally think that the "decltype" variant is harder to comprehend (and so to write properly).
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.
While I'm not sure that your alternative is "simpler", I've included it as another example for those who do.
I prefer the first version for readability, however I may be missing some advantage the second provides technically.
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.
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.
thanks, fixed.
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 meThere 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.