Unintended different behavior between SYCL and OpenCL attributes

[rolandschulz]

#define __private __attribute__((opencl_private))
#define __local   __attribute__((opencl_local))

//struct S { void f() __private; }; //should be OK, but is error

void t() {
    //__private int i1; //should be OK, but is error
    int i2;
    __private int* p;
    __local int* l;
    p-l; //should be error "non-overlapping address spaces"
}

OCL C++ version: https://godbolt.org/z/wCDKhZ

For users to use the decorated pointer (from multi_ptr::pointer_t) for performance tuning it is important this doesn't have any unintentional differences.

• Attribute on methods is important to have a this pointer with attribute
• Allowing attribute on variable avoids having to strip address space in templated code
• Error for non-overlapping address space is important to have correct code

#1581 contains some previous discussion of this issue.

[rolandschulz]

@AnastasiaStulova is it intentional that for OpenCL for C++ in most contexts __private and __attribute__((opencl_private)) behaves the same but that for methods qualifiers only the keyword works but the attribute doesn't? Example:

struct S1{ void f() __attribute__((opencl_private)); }; //error
struct S2{ void f() __private; }; 

If this is intentional what's the reason? For SYCL we only want to support the attribute but not the keyword. Do you think there is a problem for us to allow the attribute?

[AnastasiaStulova]

@rolandschulz address space attr works slightly differently in C++ outside of OpenCL. I have a patch that generalizes the behavior with the attribute spelling https://reviews.llvm.org/D57464.

I think it should work for your use case but I got quite many review comments and didn't have time to finalize this. :(

[bader]

@AnastasiaStulova, could we add one more test to https://reviews.llvm.org/D57464 to cover C++ for OpenCL mode as well? Existing test case covers non-OpenCL C++ mode only.

struct S1{ void f() __attribute__((opencl_private)); }; // should be ok
struct S2{ void f() __private; };

[AnastasiaStulova]

Just FYI C++ for OpenCL is covered by different review:
https://reviews.llvm.org/D55850
And it has been committed to Clang more than a year ago.

If you are interested in having https://reviews.llvm.org/D57464 committed too, I can't promise any timeline at the moment, unfortunately. But I am happy if you want to go ahead and rebase/rework it upstream. I will certainly be able to help you finilising it.

[bader]

Just FYI C++ for OpenCL is covered by different review:
https://reviews.llvm.org/D55850
And it has been committed to Clang more than a year ago.

Interesting...
@Naghasan added new spelling for OpenCL address space attributes a little bit less than a year ago. llvm/llvm-project@11a9bae

New spelling doesn't work as original keywords. https://godbolt.org/z/_W8xH8
Any ideas why?

I'm not sure if https://reviews.llvm.org/D57464 will help to resolve this issue.
@rolandschulz, what is the impact of this this issue?

[Naghasan]

Just FYI C++ for OpenCL is covered by different review:
https://reviews.llvm.org/D55850
And it has been committed to Clang more than a year ago.

Interesting...
@Naghasan added new spelling for OpenCL address space attributes a little bit less than a year ago. llvm/llvm-project@11a9bae

New spelling doesn't work as original keywords. https://godbolt.org/z/_W8xH8
Any ideas why?

None of the spelling are working :) I think this is a grammar problem here.

[AnastasiaStulova]

New spelling doesn't work as original keywords. https://godbolt.org/z/_W8xH8

Any ideas why?

Well clang only has support for OpenCL address spaces. Use of attribute((opencl_private)) is a Clang extension. I think though it should be pretty straight forward to extend https://reviews.llvm.org/D57464 to work for your case. Because it is also attribute-based?

[rolandschulz]

what is the impact of this this issue?

The impact of not being able to put a qualifier on C++ methods is that the this pointer is generic. Whether that in turn has an impact depends on whether we can optimize.

[bader]

Patch for "Error for non-overlapping address space is important to have correct code" - https://reviews.llvm.org/D80317.