Anastasia added a comment.

In D80932#2073542 <https://reviews.llvm.org/D80932#2073542>, @Naghasan wrote:

> In D80932#2072064 <https://reviews.llvm.org/D80932#2072064>, @Anastasia wrote:
>
> > In D80932#2071321 <https://reviews.llvm.org/D80932#2071321>, @bader wrote:
> >
> > > In D80932#2068863 <https://reviews.llvm.org/D80932#2068863>, @Anastasia 
> > > wrote:
> > >
> > > >
> > >
> > >
> > >
> > >
> > > > Why? Can you explain what you are trying to achieve with this?
> > >
> > > I think @asavonic can provide more detailed answer, but IIRC we spent a 
> > > lot time trying to marry template meta-programming with OpenCL address 
> > > space deductions, but even simplest template functions from C++ standard 
> > > library breaks compilation. It's important to note one important 
> > > difference in design goals for SYCL and C++ for OpenCL. SYCL aims to 
> > > enable compilation of regular C++ as much as possible where as one of the 
> > > C++ for OpenCL goals is to keep compatibility with OpenCL C. These two 
> > > goals might lead to different design decisions.
> >
> >
> > I don't see a contradiction in those goals. We plan to support C++ 
> > libraries with C++ for OpenCL of course with functionality that is accepted 
> > by conformant OpenCL implementations. For example, libraries that use 
> > virtual functions leading to function pointers are not going to work 
> > portably in OpenCL devices. If SYCL aims to compile to conformant OpenCL 
> > devices then it should not be very different I imagine?
>
>
>   (Edited a bit)
>   
>
> There is a fundamental difference (given the direction taken for OpenCL), 
> OpenCL mode actively modifies user's types  (C and C++ for OpenCL AFAIK). So 
> inside a function, this:
>
>   int var;
>
>
> becomes
>
>   VarDecl  var '__private int'
>
>
> Which is fine in C where you can't really do much  with your types, but this 
> has a massive and destructive effect in C++. For instance this does not 
> compile in C++ for OpenCL:
>
>   void foo() {
>       int var; // '__private int' not 'int'
>       int* ptr1 = &var; // '__generic int* __private' not 'int*'
>       decltype(var)* ptr2 = ptr1; // error: cannot initialize a variable of 
> type 'decltype(var) *__private' (aka '__private int *__private') with an 
> lvalue of type '__generic int *__private'
>   }
>
>
>


Correct. For this we plan to add a metaprogramming utility to remove address 
space from a type. Here is a proposal:
https://reviews.llvm.org/D76636

However, this would mean modifying the C++ libraries to produce a flavor that 
can be supported by OpenCL devices.

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.

>> 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. :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80932



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to