-
Notifications
You must be signed in to change notification settings - Fork 790
[libspirv][remangler] Remangle pointer address space when target's default addrspace is private #18383
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
[libspirv][remangler] Remangle pointer address space when target's default addrspace is private #18383
Conversation
…fault addrspace is private For target where default address space is private, pointer address space mangling in libclc is not compatible with SYCL mangling. Take spir64 target as example, OpenCL mangling: _Z17__spirv_ocl_fractfPU3AS4f(float, ptr addrspace(4)) _Z17__spirv_ocl_fractfPf(float, ptr) SYCL mangling: _Z17__spirv_ocl_fractfPf(float, ptr addrspace(4)) _Z17__spirv_ocl_fractfPU3AS0f(float, ptr) This leads to issue when linking libclc built-ins to SYCL device code. This PR fixes the issue by remangling libclc built-ins to align with SYCL: Remangle _Z17__spirv_ocl_fractfPU3AS4f to _Z17__spirv_ocl_fractfPf. Remangle _Z17__spirv_ocl_fractfPf to _Z17__spirv_ocl_fractfPU3AS0f. Relates to intel#16703
kindly ping @intel/llvm-reviewers-cuda for review, thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any tests we can add for this?
Which targets does this alter codegen for?
// revert _Z1fPi$TmpSuffix back to _Z1fPi, a clash occurs because _Z1fPi | ||
// still exists. Since _Z1fPi is no longer useful, it is renamed to | ||
// _Z1fPi.old, | ||
if (auto *Func = M->getFunction(Name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we think about removing this function instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, deleted the function
This PR is for targets whose default addrspace is private, such as spir64/spirv. |
@@ -271,6 +285,7 @@ class Remangler { | |||
: AST(AST), Root(Root), TypeReplacements(TypeReplacements) { | |||
MangleContext.reset( | |||
ItaniumMangleContext::create(*AST, AST->getDiagnostics())); | |||
TargetGenericAddrSpace = AST->getTargetAddressSpace(LangAS::Default); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be called TargetDefaultAddrSpace
? I worry that 'generic' is an overloaded term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be called
TargetDefaultAddrSpace
? I worry that 'generic' is an overloaded term.
done, thanks for the suggestion
CI is broken. Considering the latest commit just renamed the variable and before the renaming CI was green, it's safe to merge. |
For target where default address space is private, pointer address space mangling in libclc is not compatible with SYCL mangling. Take spir64 target as example,
OpenCL mangling:
_Z17__spirv_ocl_fractfPU3AS4f(float, ptr addrspace(4))
_Z17__spirv_ocl_fractfPf(float, ptr)
SYCL mangling:
_Z17__spirv_ocl_fractfPf(float, ptr addrspace(4))
_Z17__spirv_ocl_fractfPU3AS0f(float, ptr)
This leads to issue when linking libclc built-ins to SYCL device code. This PR fixes the issue by remangling libclc built-ins to align with SYCL: Remangle _Z17__spirv_ocl_fractfPU3AS4f to _Z17__spirv_ocl_fractfPf. Remangle _Z17__spirv_ocl_fractfPf to _Z17__spirv_ocl_fractfPU3AS0f.
Relates to #16703
This PR is for targets whose default addrspace is private, such as
spir64/spirv. In intel/llvm repo, currently there is no such libspirv target.
In our downstream repo, we have a target of this kind.
Put this PR in this repo to avoid customization in the downstream repo.