[PATCH] D80932: [SYCL] Make default address space a superset of OpenCL address spaces.

[Alexey Bader via Phabricator via cfe-commits]

bader marked an inline comment as done.
bader added a comment.

In D80932#2074792 <https://reviews.llvm.org/D80932#2074792>, @Anastasia wrote:

> I think my biggest problem is that I don't understand how you can just run C++ libraries on OpenCL devices. If we were able to compile plain C++ for OpenCL devices would we not just do it? But OpenCL devices have certain constraints. Also aren't libraries typically customized for performance depending on the range of HW they are being run on? I see the modifications are inevitable.


The approach we use won't enable all C++ libraries, but it unblocks template metaprogramming patterns which conceptually are valid code. We can use STL parts of C++ standard library without re-writing them, which is a huge win for C++ developers.

We employ address space inference pass (https://llvm.org/doxygen/InferAddressSpaces_8cpp_source.html) to improve the performance of C++ libraries. Users can help the compiler analysis and optimize code by using SYCL `multi_ptr` template class specialized with particular memory region.

>>> We plan to support C++ libraries with C++ for OpenCL
>> 
>> With the direction taken so far, C++ for OpenCL can't properly implement or use basic C++ due to this.Small example using a `is_same` implementation (https://godbolt.org/z/CLFV6z):
>> 
>>   template<typename T1, typename T2>
>>   struct is_same {
>>       static constexpr int value = 0;
>>   };
>>   
>>   template<typename T>
>>   struct is_same<T, T> {
>>       static constexpr int value = 1;
>>   };
>>   
>>   void foo(int p) {
>>       static_assert(is_same<decltype(p), int>::value, "int is not an int?"); // Fails: p is '__private int' != 'int'
>>       static_assert(is_same<decltype(&p), int*>::value, "int* is not an int*?");  // Fails: p is '__private int*' != '__generic int*'
>>   }
>> 
>> 
>> So yes, you could implement `std::is_same` but it won't work as one would expect.
>> 
>> That's the reason why SYCL actively tries to prevent changing any type, this would prevent the compilation of valid C++ code without a fundamental reason (e.g. the target is not able to support it).
>> 
>> My point is only that the approach taken for C++ for OpenCL is not suitable for SYCL IMO
>> 
>>> Default address space is a Clang concept that is generally used for an automatic storage.
>> 
>> That's not true in CUDA. On the other hand they actively avoids using address space.
> 
> True and I don't think CUDA uses `isAddressSpaceSupersetOf` in their implementation? Perhaps SYCL can use the same flow?
> 
>> Plus in `isAddressSpaceSupersetOf` you have:
>> 
>>   // Consider pointer size address spaces to be equivalent to default.
>>   ((isPtrSizeAddressSpace(A) || A == LangAS::Default) &&
>>               (isPtrSizeAddressSpace(B) || B == LangAS::Default))
>> 
>> 
>> An alternative would be to clone the address space. But that would be shame to not find a way to reuse the opencl one as down the line, they map to the same thing.
> 
> I agree that reusing functionality is preferable however it is subject to whether it is sufficiently similar. If we end up adding a lot of code to diverge special cases or even worse regress existing functionality by adding new one then perhaps this is not a viable solution.
> 
> The problem is that I understand very little of the design at this point to suggest anything. All I know is that there are magic pointer classes in SYCL spec that represent address spaces somehow but what I see in this review is a change to OpenCL address space model that isn't governed by any spec or explained in any documentation. Perhaps it is easy and transparent to you but I see very little correlation. :(

@Anastasia, if we make this change specific to SYCL mode, will it address your concerns?

If not, we will go with the alternative proposed by Victor.
I'd like to mention that just a few hours ago we accepted a PR extending address space attributes in SYCL mode to enable efficient code generation of the code using USM extension (Unified Shared Memory) on targets w/o hardware support for this feature. I think this might be relevant information to take a decision.



================
Comment at: clang/include/clang/AST/Type.h:493
+                                    other.getAddressSpace()) ||
+           (!hasAddressSpace() &&
+            (other.getAddressSpace() == LangAS::opencl_private ||
----------------
Naghasan wrote:
> Not sure how to make it better, but this may not be true depending on what is allowed by the language.
You are right. For some reason I was sure that these address spaces are available only in SYCL and OpenCL modes. I update the patch to enable this conversion for SYCL mode only to avoid any impact on OpenCL mode.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80932/new/

https://reviews.llvm.org/D80932