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
[SYCL] Select device image based on compile_target device image property #14909
[SYCL] Select device image based on compile_target device image property #14909
Changes from 6 commits
813f34b
2d1833d
5d51eb2
7123d94
0cfd251
2e3e327
9b71ea2
aa46482
d35e140
9596118
d41d5f0
9b21eae
671f228
ae98cdb
3a59e70
dce0da9
57d33d0
46726be
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.
There is
asCString
helper:llvm/sycl/source/detail/device_binary_image.hpp
Line 76 in 8e6d451
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.
Is the property data guaranteed to be null terminated though? From my understanding the memory layout of these property values come from the
PropertyValue
class, which I don't think explicitly null terminates the data.llvm/llvm/include/llvm/Support/PropertySetIO.h
Lines 84 to 86 in cec1423
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.
Good point. I still think that using
asCString
would be a better fit, but I assume it requires an update to the helper itself to be made safe first. Therefore, that seems like an improvement that can/should be done separatelyThere 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.
Am I right that this assert is effectively replaced by an assert within
vector::operator[]
?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 removed this assert because it is now not guaranteed the
Img
will be non null in this case. Specifically, there could be no image compatible with the current device, andgetBinImageFromMultiMap
will return nullptr:https://github.com/intel/llvm/pull/14909/files#diff-78dd7f7ba0b6120dece1ae4ab5a09c9936ff654a1de2c31ff2dbb1fc58d90393R1308-R1309
In this case, the control flow then flows down to the exception. This flow is demonstrated by the
NoDeviceKernel
test.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.
Shouldn't this be alphabetically sorted? Not sure really, some of the elements don't seem to be.
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 wonder if this exception message should be improved, because it can be quite confusing. A kernel may be right in front of a developer, but it is not that we couldn't find it, it is our program is not compatible with a device because of how it was compiler.
I suppose that this error message is what we will see even without this patch, so I'm fine with addressing that separately.