Skip to content
This repository was archived by the owner on Mar 28, 2023. It is now read-only.

[SYCL] Add an OpenCL function caching issue workaround to get_backend #693

Closed
wants to merge 3 commits into from
Closed

[SYCL] Add an OpenCL function caching issue workaround to get_backend #693

wants to merge 3 commits into from

Conversation

sergey-semenov
Copy link

@sergey-semenov sergey-semenov commented Dec 29, 2021

There is an issue with extension function pointer caching in OpenCL
plugin: each extension function call helper keeps a pi_context-to-fptr
map cache, however, these maps aren't cleaned up on context release. This
can lead to the following situation:

  • Context A is created
  • An extension function is called in context A
  • Context A is destroyed
  • A new context B is created, which happens to have the same pi_context
    handle
  • The cached function is called for context B

If these two contexts have different platforms (like in get_backend
test), this will lead to an error. This patch removes the use of USM
functions from the test to circumvent this issue and reenable
get_backend API testing.

There is an issue with extension function pointer caching in OpenCL
plugin: each extension function call helper keeps a pi_context-to-fptr
map cache, however, these maps aren't cleaned up on context release. This
can lead to the following situation:
- Context A is created
- An extension function is called in context A
- Context A is destroyed
- A new context B is created, which happens to have the same pi_context
handle
- A cached function is called

If these two contexts have different platforms (like in get_backend
test), this will lead to an error. This patch removes the use of USM
functions from the test to circumvent this issue and reenable
get_backend API testing.
@sergey-semenov sergey-semenov requested a review from a team as a code owner December 29, 2021 09:53
@sergey-semenov
Copy link
Author

It seems that clang-format crashes because of the deletion of the first line in the file. If I recall correctly, this is a known problem.

Copy link

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

As a work-around this looks good to me. Should we have a comment about it being a work-around or should we keep it after the problem has been fixed and let another - more specialized - test handle the testing?

@sergey-semenov
Copy link
Author

Superseded by #745 & intel/llvm#5282

@s-kanaev s-kanaev reopened this Feb 1, 2022
@vladimirlaz
Copy link

@s-kanaev, @sergey-semenov do we need this PR? If so could you please address the CI issue?

@vladimirlaz
Copy link

@sergey-semenov, @s-kanaev, do we need this PR?

@s-kanaev
Copy link

s-kanaev commented Mar 1, 2022

Yes. This PR is needed. I'll handle CI failures.

@vladimirlaz vladimirlaz marked this pull request as draft March 16, 2022 07:05
@vladimirlaz
Copy link

Moving to Draft until CI failure are fixed

@sergey-semenov sergey-semenov closed this by deleting the head repository Aug 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants