Skip to content
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] Remove plugin interface #14145

Merged
merged 239 commits into from
Jul 26, 2024
Merged

[SYCL] Remove plugin interface #14145

merged 239 commits into from
Jul 26, 2024

Conversation

aarongreig
Copy link
Contributor

No description provided.

@kbenzie kbenzie changed the title Draft: Remove plugin interface Draft: [ABI-Break] Remove plugin interface Jun 14, 2024
@kbenzie kbenzie added the abi-break change that's breaking abi and waiting for the next window to be able to merge label Jun 14, 2024
aarongreig and others added 27 commits June 17, 2024 15:55
Also fix discarded result for mem allocations, and finish device global handling
This was mostly done anyway.
Copy link
Contributor

@dkhaldi dkhaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am okay with disabling the 5 joint matrix tests for the sake of this PR since the plan @kbenzie shared with me is that he will reenable them in a separate PR after he fixes the remaining issues.

@kbenzie
Copy link
Contributor

kbenzie commented Jul 26, 2024

@intel/llvm-gatekeepers please merge

///
/// \ingroup sycl_pi

#pragma once
Copy link

@gajanan-choudhary gajanan-choudhary Aug 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file was being included in at least one external software, the open-source oneMKL Interfaces project, which breaks after removal of this file. Can someone please comment why this PR is not marked as a breaking change unlike some others? Also, it would help to know what the fix would be to oneMKL Interfaces here to prevent errors like:

/export/users/gchoudha/oneMKL-interfaces/src/blas/backends/cublas/cublas_scope_handle.hpp:31:10: fatal error: 'sycl/detail/pi.hpp' file not found
   31 | #include <sycl/detail/pi.hpp>
      |          ^~~~~~~~~~~~~~~~~~~~
1 error generated.

Copy link
Contributor

@AlexeySachkov AlexeySachkov Aug 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pi.hpp is an undocumented header which is not a part of our public API and we do not guarantee stability of internal APIs.

AlexeySachkov added a commit to AlexeySachkov/llvm that referenced this pull request Sep 24, 2024
This is a by-product of intel#14145. Functions to dynamically load a library and
query a symbol out of it should not be attached to UR, because they are used for
other libraries as well.

Moved them from `detail::ur` into `detail` namespace, outlined into a separate
header and removed declarations from public SYCL headers.

Fixes intel#14923
steffenlarsen pushed a commit that referenced this pull request Oct 7, 2024
These e2e tests started failing after PI removal and replacing it with
UR ([PR](#14145)) However, they were
not related to it.

On Windows, dlls unloading is inconsistent and if we try to release
these UR objects manually, inconsistent hangs happen due to a race
between unloading the UR adapters dlls (in addition to their dependency
dlls) and the releasing of these UR objects (The proxy loader have
solved this problem partially
[here](#15262)). So, we currently
shutdown without releasing them and windows should handle the memory
cleanup.

This behaviour is the same old behaviour as before removing PI as on
Investigations on this. This was only hidden before PI removal as it was
calling the PI entry-point but doesn't make it to UR entry-point and
Filecheck logs check for objects release would pass as it only check for
the call to the PI entry-point without check that the call was a
successful call. That was the PI call for `piContextRelease` before PI
removal:

```
---> piContextRelease(
        <unknown> : 0000023CC0EBA6C0
) ---> API Called After Plugin Teardown, Functon Call ignored.
```

Fixes #14768
Fixes #14950
Fixes #14968
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.