-
Notifications
You must be signed in to change notification settings - Fork 724
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][Bindless][ABI-Break] Remove deprecated functions and structs #14555
[SYCL][Bindless][ABI-Break] Remove deprecated functions and structs #14555
Conversation
* remove the following deprecated functions and their overloads: - read_image - read_mipmap - map_external_memory_array - free_mipmap_mem - alloc_mipmap_mem - free_image_mem * removed deprecated structures in bindless_images_interop.hpp * removed piextMemImportOpaqueFD and piextImportExternalSemaphoreOpaqueFD
friendly ping. @intel/dpcpp-nativecpu-pi-reviewers, @intel/llvm-reviewers-cuda, @intel/llvm-reviewers-runtime, @MartinWehking and @steffenlarsen. 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.
Native CPU lgtm, thank you
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.
UR LGTM
friendly ping @intel/llvm-reviewers-cuda, @intel/llvm-reviewers-cuda, @MartinWehking and @steffenlarsen. 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
friendly ping @intel/llvm-reviewers-runtime and @steffenlarsen. thanks |
Sorry, this one flew under my radar! Due to this touching PI interfaces, I suspect it will conflict strongly with #14145. Since the ABI breaks related to this is in experimental interfaces, it is not strictly restricted by the ABI-breaking window, so I would prefer that we prioritize #14145 getting merged. |
I'm not sure we're able to skirt the ABI restrictions, though. Because if we remove symbols from the SYCL library, that breaks ABI for all users, not just ones using Bindless Images. So that means if we don't get the patches in this window, we need to wait until the next major release. We realize #14145 is very important, however all of the Bindless Images patches have been thoroughly reviewed and are basically ready to merge. One option that we could do is to collate all of our patches into one big PR, that should reduce potential conflicts with #14145 , though I'm a bit skeptical about collecting approvals for everything again. Do you think it's worth doing that? |
Feel free to prove me wrong, but I do not believe this is correct. As long as an application does not use these symbols, they should not be affected by these symbols disappearing. Keep in mind, it is not just my opinion that experimental ABI-breaking changes are allowed outside ABI-breaking windows, it is mentioned in https://github.com/intel/llvm/blob/sycl/sycl/doc/developer/ABIPolicyGuide.md#changing-abi. Of course, this does not mean we should make them all the time, but it means for changes like these we are not as heavily restricted.
I understand that, but based on what I mention above, #14145 is fighting time more than this patch is. I do not believe that combining them into a single PR will do much to aid the conflict issue, apart from maybe making it fewer iterations for the maintainers in #14145. I suspect rebasing these will only really require re-review from runtime reviewers, so I will happily prioritize doing these re-reviews if you ping me on them or send me a list of PRs. |
Tag @aarongreig & @kbenzie - If you are okay with these proceeding, we can proceed, but there will be more conflicts to resolve in #14145 if we do. Same applies to #14444 and #14140. |
@steffenlarsen thanks for pointing that out. It should be resolved now. |
@intel/llvm-gatekeepers This can be meged. Thanks. |
@cppchedy LGTM |
remove the following deprecated functions and their overloads:
remove deprecated structures in
bindless_images_interop.hpp
remove piextMemImportOpaqueFD and piextImportExternalSemaphoreOpaqueFD