-
Notifications
You must be signed in to change notification settings - Fork 787
[SYCL] Make SYCL RT compatible with the new offload entry type #17109
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
@@ -1869,31 +1869,32 @@ void ProgramManager::addImages(sycl_device_binaries DeviceBinary) { | |||
for (sycl_offload_entry EntriesIt = EntriesB; EntriesIt != EntriesE; |
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.
will the iterator work when the input binary is using legacy entry format? sizeof entries are different.
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.
LGTM overall
++EntriesIt) { | ||
sycl_offload_entry EntriesIt; | ||
auto IncrementEntriesIt = [&]() { | ||
if (EntriesIt->IsNewOffloadEntryType()) |
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 shouldn't expect a mix of old and new entries, so I think this check should be made once instead of on each increment.
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.
Done in 8d70601
@intel/dpcpp-clang-driver-reviewers @intel/dpcpp-tools-reviewers please review the changes. Thanks. |
Co-authored-by: Sergey Semenov <[email protected]>
Thanks so much for putting this change up. I will add my comments (if any) soon. |
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.
Overall looks good. Minor nits, some of which might need addressing.
Thanks
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.
LGTM. Thanks for the updates.
…#17109) llvm/llvm-project#124018 change the type of offload struct. This PR makes SYCL RT compatible with the new format, while keeping backwards compatibility with the older version (till next ABI-break window). --------- Co-authored-by: Sergey Semenov <[email protected]>
llvm/llvm-project#124018 change the type of offload struct. This PR makes SYCL RT compatible with the new format, while keeping backwards compatibility with the older version (till next ABI-break window).