-
Notifications
You must be signed in to change notification settings - Fork 787
[SYCL] Implement loading SYCLBIN into kernel_bundle #18949
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] Implement loading SYCLBIN into kernel_bundle #18949
Conversation
This commit implements the functionality for loading SYCLBIN files into kernel bundles. This is done by mimicing the structure of regular device binaries, then letting the existing functionality handle compiling and linking. This implements part of the sycl_ext_oneapi_syclbin extension. Signed-off-by: Larsen, Steffen <[email protected]>
Signed-off-by: Larsen, Steffen <[email protected]>
Signed-off-by: Larsen, Steffen <[email protected]>
Signed-off-by: Larsen, Steffen <[email protected]>
Signed-off-by: Larsen, Steffen <[email protected]>
Signed-off-by: Larsen, Steffen <[email protected]>
Signed-off-by: Larsen, Steffen <[email protected]>
Signed-off-by: Larsen, Steffen <[email protected]>
Signed-off-by: Larsen, Steffen <[email protected]>
Signed-off-by: Larsen, Steffen <[email protected]>
Signed-off-by: Larsen, Steffen <[email protected]>
@@ -463,12 +464,27 @@ class kernel_bundle_impl | |||
"Not all input bundles have the same set of associated devices."); | |||
} | |||
|
|||
// Pre-count and reserve space in vectors. |
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.
question: Is constant time access to all three of these really important?
If it is, let's continue with std::vector
. But if it isn't, maybe we should just use std::list
? With std::list
we don't need to "reserve", we get constant time insertion and deletion. Deleting/erase an element doesn't cause move
on the other elements. There are a lot of advantages. Of course, lookup, even by index, becomes linear time instead of constant time.
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.
They are occasionally all iterated over, which is faster with std::vector
. std::list
would indeed have constant selective deletion, insertion and preservation of memory, but these collections should not change after creation. The merging is only done when joining/linking kernel bundles, as the results are the combination of all the kernel bundles.
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.
Looks awesome! 🎉
Just some non-blocking comments which can be addressed in a separate PR
reinterpret_cast<const OffloadBinaryEntryType *>(Data + | ||
Header->EntryOffset); | ||
|
||
if (Entry->ImageKind != /*IMG_SYCLBIN*/ 6) |
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.
Should we move this constant somewhere? Where did 6
come from?
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's sadly a little bit of a hack, stemming from https://github.com/intel/llvm/blob/sycl/llvm/include/llvm/Object/OffloadBinary.h#L50. Eventually this file will disappear in favor of the LLVMObject version of SYCLBIN parsing, but for now we can't access that enum. Do you think it would make sense to make a constexpr uint16_t IMG_SYCLBIN = 6;
and use that? I'm not sure how much it adds as it's still just a magic value, but maybe it's easier to make the intention clear. Alternatively, I can add a comment.
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 you think this value will be used somewhere else in this temporary file? If not, I guess this is fine to keep as is, otherwise constexpr variable is good to be added, but overall this is nit
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 probably want to use it when we repack eventually too, but I hope we can have these temps gone by that time.
{ | ||
std::ifstream FileStream{Filename, std::ios::binary}; | ||
if (!FileStream.is_open()) | ||
throw sycl::exception(make_error_code(errc::invalid), |
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 spec:
_Throws:_
* A `std::ios_base::failure` exception if the function failed to access and read
the file specified by `filename`.
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.
Ah, good point!
Signed-off-by: Larsen, Steffen <[email protected]>
…bundle_impl` Follow-up for intel#18899. Also adds proper `private_tag` argument for the ctor added in intel#18949 that missed that.
…bundle_impl` Follow-up for intel#18899. Also adds proper `private_tag` argument for the ctor added in intel#18949 that missed that. Also pass `span` by value while on it.
This commit implements the functionality for loading SYCLBIN files into kernel bundles. This is done by mimicking the structure of regular device binaries, then letting the existing functionality handle compiling and linking.
This implements part of the sycl_ext_oneapi_syclbin extension.
Note that parts of this implementation uses functionality copied from LLVMSupport and LLVMObject. Eventually they should be replaced in favor of using the LLVM libraries directly.