Skip to content

[SYCL] Fix get_native() and add get_backend() accordingly to the specification and test for the get_native() #3236

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
merged 25 commits into from
Mar 12, 2021

Conversation

maximdimakov
Copy link
Contributor

@maximdimakov maximdimakov commented Feb 20, 2021

Free get_native() method must call retain method for some classes when backend is opencl accordingly to the specification.
Also for these classes there must be get_backend() method.
Test for get_backend() : intel/llvm-test-suite#162

@maximdimakov maximdimakov requested a review from a team as a code owner February 20, 2021 14:11
@maximdimakov maximdimakov requested a review from s-kanaev March 2, 2021 06:47
@maximdimakov maximdimakov changed the title [SYCL] Fix get_native() and add get_backend() accordingly to the specification [SYCL] Fix get_native() and add get_backend() accordingly to the specification and test for the get_native() Mar 3, 2021
Copy link
Contributor

@s-kanaev s-kanaev left a comment

Choose a reason for hiding this comment

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

Looks good to me.
@smaslov-intel, could you, please, review. I believe, you are more common with interoperability than me.

@alexbatashev alexbatashev self-requested a review March 5, 2021 09:35
@maximdimakov
Copy link
Contributor Author

@alexbatashev , @smaslov-intel can you review, please?

Copy link
Contributor

@alexbatashev alexbatashev left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd like @smaslov-intel to approve.

Signed-off-by: mdimakov <[email protected]>
smaslov-intel
smaslov-intel previously approved these changes Mar 10, 2021
Copy link
Contributor

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

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

LGTM

smaslov-intel
smaslov-intel previously approved these changes Mar 11, 2021
@bader bader merged commit ee7e99f into intel:sycl Mar 12, 2021
@@ -3897,6 +3897,7 @@ _ZNK2cl4sycl15interop_handler12GetNativeMemEPNS0_6detail16AccessorImplHostE
_ZNK2cl4sycl15interop_handler14GetNativeQueueEv
_ZNK2cl4sycl16default_selectorclERKNS0_6deviceE
_ZNK2cl4sycl20accelerator_selectorclERKNS0_6deviceE
_ZNK2cl4sycl5event11get_backendEv
Copy link
Contributor

Choose a reason for hiding this comment

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

Should minor/patch version of library be increased according to ABI policy?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it'll be increased before release is tagged.

aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Feb 25, 2023
OpenCL interop spec basically says that each "conversion" must adjust
reference counter of the underlying object. Many of the interop
interfaces had been fixed in intel#3236 but
the majority of the kernel_bundle feature was implemented after that and
made the same omission.

I'm not sure if that is the best way to fix it though. Another
alternative would be to move all these retains into the OpenCL PI
plugin.
bader pushed a commit that referenced this pull request Mar 2, 2023
OpenCL interop spec basically says that each "conversion" must adjust
reference counter of the underlying object. Many of the interop
interfaces had been fixed in #3236 but
the majority of the kernel_bundle feature was implemented after that and
made the same omission.

I'm not sure if that is the best way to fix it though. Another
alternative would be to move all these retains into the OpenCL PI
plugin.
iclsrc pushed a commit that referenced this pull request Jun 17, 2025
Update a test after llvm-project commit 28bda77 ("Introduce
MCAsmInfo::UsesSetToEquateSymbol and prefer = to .set", 2025-06-11).

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@46f225a7d0aa111
iclsrc pushed a commit that referenced this pull request Jun 30, 2025
Update a test after llvm-project commit 28bda77 ("Introduce
MCAsmInfo::UsesSetToEquateSymbol and prefer = to .set", 2025-06-11).

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@46f225a7d0aa111
mikolaj-pirog pushed a commit that referenced this pull request Jul 1, 2025
Update a test after llvm-project commit 28bda77 ("Introduce
MCAsmInfo::UsesSetToEquateSymbol and prefer = to .set", 2025-06-11).

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@46f225a7d0aa111
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.

5 participants