Skip to content

[SYCL][ABI-Break] Drop pi::die #14626

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

Merged

Conversation

AlexeySachkov
Copy link
Contributor

@AlexeySachkov AlexeySachkov commented Jul 18, 2024

It is not used from SYCL headers, there is no reason for it to be exported and moreover it only has a single use since pi::assertion was removed.

This is patch is essentially a by-product of #14145 and it is done to simplify that change, i.e. PI plugins removal should not be ABI-breaking by itself, we just need to cleanup some of our exported symbols.

It is not used from SYCL headers, there is no reason for it to be
exported.
@AlexeySachkov AlexeySachkov added the abi-break change that's breaking abi and waiting for the next window to be able to merge label Jul 18, 2024
@AlexeySachkov AlexeySachkov requested a review from a team as a code owner July 18, 2024 13:56
Copy link
Contributor

@aelovikov-intel aelovikov-intel left a 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 how we can be sure it isn't used on some code path that isn't instantiated in our tests.

@AlexeySachkov
Copy link
Contributor Author

I don't know how we can be sure it isn't used on some code path that isn't instantiated in our tests.

You mean someone uses it directly in their application? I did grep for every endpoint that I'm hiding to find if it is used and that's what I'm basing my conclusion that it is unused on

@aelovikov-intel
Copy link
Contributor

aelovikov-intel commented Jul 18, 2024

I don't know how we can be sure it isn't used on some code path that isn't instantiated in our tests.

You mean someone uses it directly in their application? I did grep for every endpoint that I'm hiding to find if it is used and that's what I'm basing my conclusion that it is unused on

No, only internal (current and future) uses. In other words, I don't like that there is implicit dependency of not using this inside our header resulting in the symbol used in user's program.

To clarify - if the symbol is only used under source/, then declaration should happen under a header in that folder, not in public headers.

@AlexeySachkov
Copy link
Contributor Author

I don't know how we can be sure it isn't used on some code path that isn't instantiated in our tests.

You mean someone uses it directly in their application? I did grep for every endpoint that I'm hiding to find if it is used and that's what I'm basing my conclusion that it is unused on

No, only internal (current and future) uses. In other words, I don't like that there is implicit dependency of not using this inside our header resulting in the symbol used in user's program.

To clarify - if the symbol is only used under source/, then declaration should happen under a header in that folder, not in public headers.

Thanks, I see, that makes sense. It is also used by plugins, but I suppose that I can define it in every plugin as well as a temporary measure until they are deleted

@AlexeySachkov
Copy link
Contributor Author

AlexeySachkov commented Jul 18, 2024

To clarify - if the symbol is only used under source/, then declaration should happen under a header in that folder, not in public headers.

Thanks, I see, that makes sense. It is also used by plugins, but I suppose that I can define it in every plugin as well as a temporary measure until they are deleted

Plugins currently use their own definition of die which they bring from UR by including pi2ur.hpp. Therefore my plan is to completely remove the function. Its only use is to report an unsupported platform type when we convert it to string, which I believe we can handle simply by reporting a special string saying that the platform is unknown. SYCL error reporting mechanism is exceptions and if we need something else, we can re-introduce it.

pi::assertion uses pi::die, so #14624 should go first

@AlexeySachkov AlexeySachkov changed the title [SYCL][ABI-Break] Hide pi::die symbol [SYCL][ABI-Break] Drop pi::die Jul 19, 2024
@AlexeySachkov AlexeySachkov merged commit 1891dc3 into intel:sycl Jul 19, 2024
15 checks passed
@AlexeySachkov AlexeySachkov deleted the private/asachkov/hide-pi-die-symbol branch October 9, 2024 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abi-break change that's breaking abi and waiting for the next window to be able to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants