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

Revert "Turn on SPIR-V builtin generation for OpenCL CPP sources (#2466)" #2508

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

bwlodarcz
Copy link
Contributor

This reverts commit 80dfd86. The commit breaks Translator in cases when user supplied function is named with the same prefix, after demangling, as OpenCL builtin. The culprit code is in SPIRWriter OCL pass in function OCLToSPIRVBase::visitCallInst(CallInst &).
Failing name would be for example atomic_fetch_and_sub_uint32_explicit.

…onosGroup#2466)"

This reverts commit 80dfd86.
The commit break Translator in cases when user supplied function is named
with the prefix which, after demangling, is the same as OpenCL builtin.
The culprit code is placed in during SPIRWriter OCL pass in
OCLToSPIRVBase::visitCallInst(CallInst &).
Failing name would be for example atomic_fetch_and_sub_uint32_explicit.
Copy link
Member

@svenvh svenvh left a comment

Choose a reason for hiding this comment

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

Given that #2512 still doesn't solve the problem for user function declarations, I think we should proceed with the revert and think of a more robust solution to #2513.

@asudarsa
Copy link
Contributor

I am trying to check if this is a OpenCL C++ specific problem or if this issue can be reproduced in OpenCL C. Will revert once confirmed as a OpenCL C++ specific problem. Thanks

@asudarsa
Copy link
Contributor

Hi @svenvh,

We tested my change #2512 and all our existing test failures are resolved. This particular change (OCLToSPIRV for OpenCL C++) is important for one of our compilation flows. I would prefer to keep this in and continue discussion about covering user-declared functions.
Please let me know if that' acceptable.

Thanks

@MrSidims
Copy link
Contributor

is important for one of our compilation flows

Can it be moved to downstream then?

@MrSidims MrSidims merged commit 9e60105 into KhronosGroup:main Apr 19, 2024
9 checks passed
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.

4 participants