-
Notifications
You must be signed in to change notification settings - Fork 761
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
WIP: [SYCL] Implement SYCL address space attributes handling #968
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,12 @@ enum class LangAS : unsigned { | |
cuda_constant, | ||
cuda_shared, | ||
|
||
// SYCL specific address spaces. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remind me why we define these SYCL address spaces, and not use the OpenCL ones? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I experimented in this direction and existing rules for OpenCL address spaces prohibit casts between qualified and unqualified pointers. I can share my patch if you are interested. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI: current status of experiment with using OpenCL address spaces can be found in my private fork. |
||
sycl_global, | ||
sycl_local, | ||
sycl_constant, | ||
sycl_private, | ||
|
||
// Pointer size and extension address spaces. | ||
ptr32_sptr, | ||
ptr32_uptr, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2285,7 +2285,7 @@ void CXXNameMangler::mangleQualifiers(Qualifiers Quals, const DependentAddressSp | |
if (Context.getASTContext().addressSpaceMapManglingFor(AS)) { | ||
// <target-addrspace> ::= "AS" <address-space-number> | ||
unsigned TargetAS = Context.getASTContext().getTargetAddressSpace(AS); | ||
if (TargetAS != 0) | ||
if (TargetAS != 0 || (Context.getASTContext().getLangOpts().SYCLIsDevice)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't you know why we need this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Default address space is supposed to be akin to generic, i.e. it should be mapped to 4. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, my previous reply is not quite accurate. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, that makes sense.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have similar problem for OpenCL targeting SPIR. You seemed to use some other target your example. X86? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, the problem I'm talking about is mangling, not the mangled names conflict. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right, sorry for the confusion. With
I don't quite understand what the problem is. Can provide a test case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test case is added to this PR already: https://github.com/intel/llvm/pull/968/files#diff-9cbf615457fd73dff7e9840ed855fa9a and problem is covered in this thread. I just clarified why it affects only SYCL compiler, but not OpenCL. template<typename T>
void tmpl(T t){}
__attribute__((opencl_private)) int *PRIV;
tmpl(PRIV);
int *NoAS;
tmpl(NoAS); SYCL compiler produces the same mangled name for two I think original patch from @erichkeane mapped |
||
ASString = "AS" + llvm::utostr(TargetAS); | ||
} else { | ||
switch (AS) { | ||
|
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.
Hmm, maybe it would be good to add a comment explaining why we allowed casting between "empty" address space and others. At least for casts to
sycl_constant
address spaces.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.
@asavonic, can you help with that?
I don't think this casts should be allowed as they are not legal in OpenCL env.
Another option would be skipping
sycl_constant
in this patch.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.
Agree.