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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
