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
Initial support for ext_oneapi_composite_device. #12178
Initial support for ext_oneapi_composite_device. #12178
Changes from 10 commits
abab8b3
a3bc3e6
c559036
e5d70ea
26f9669
88b1017
28652e6
83bc72b
1f458be
031d0d9
8d68661
c866676
d7dd2b2
b5bba6a
4ad0840
e0e1b68
fe7dbdc
4e78c90
8b3d6c9
ee5c8fb
fd6bdbe
27faf1c
cefad83
b25e46e
daa047c
0816f64
81a9a2c
7c06067
43dc4fa
5b39de1
a1f6b09
110b757
6ffce19
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.
From what I tested during development of this patch, I'd say this is not used. It seems that we're now using the equivalent in URT repo.
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 will have to go away before this can be formally approved.
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.
Yes, definitely, it is just to make sure the CI uses the correct UR version.
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.
nit: we have an
std::hash<device>
specialization, so I'd imaginestd::set
could work here. Feel free to ignore.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.
Updated in 8d68661.
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.
That can be an
std::copy_if(Composites.begin(), Composites.end(), std::back_inserter{Result}, [](...) { /* predicate */ });
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.
Updated in 8d68661.
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 do we have to hardcode that?
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.
Well, we don't have to, but we know this extension only works for L0 backend, so we can save the call to PI just by checking this.
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 don't like this, but I can live with that. Please add a comment that this is just a performance optimization though. Same below.
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.
Removed in 8d68661.
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 not throwing an exception?
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.
From the extension specification:
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.
Do we normally do that? Should it be an exception instead? Isn't exit code automatically turned into an exception in
plugin::call<>
?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 don't know if we normally do that, honestly. I didn't make it an exception because this is not a fatal error, in the sense that the application could continue just returning
{}
, but it's true that it should never happen, so maybe it's better to throw.Regarding
plugin::call<>
, what we are checking here is the value ofResult
, which is not the error code. The error code is indeed checked and turned into an exception if different thanPI_SUCCESS
.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.
Changed to exception in 8d68661.