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

Wed Jun 3 13:11:13 PDT 2020

Anastasia requested changes to this revision.
Anastasia added a comment.
This revision now requires changes to proceed.

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?

C++ for OpenCL and address spaces in C++ in general is still WIP and it is not impossible that there are parts that don't work as expected yet. Have you made an analysis of issues that you found with the templates and whether it is something that can't work conceptually or is just missing from implementation? Do you have a list of those issues somewhere?

>   As we still wanted to re-use OpenCL address spaces for mapping explicitly annotated pointers to SPIR-V address spaces we decided to adopt alternative approach (inspired by OpenMP and CUDA modes) with using C++ Sema + additional semantic for non-standard attributes (like address spaces - e.g. https://reviews.llvm.org/D80317) and types. Address space deduction required by SPIR-V specification is implemented CodeGen module and it takes < 200 lines in a few files.

Address spaces in OpenCL follow certain concept and if you believe it doesn't suit your goals then the right thing to do is to start from describing and discussing your design.

> 
> 
>> Default address space is normally an address space of automatic storage i.e. private in OpenCL. If you look at the address space map of targets most of them map default and private address spaces to the same value. Converting between private and other named address space is not allowed in OpenCL see section 6.5. I don't believe this change is doing anything legitimate.
> 
> AFAIK, OpenCL compiler deduce address space at Sema for all types of storage and even automatic (https://godbolt.org/z/-5QoBd).

This is not the only example for an automatic storage.

> This change is legitimate for OpenCL because OpenCL doesn't use default address space and therefore it should be a non-functional change for OpenCL mode. I had to move on check from the OpenCL C++ test because this changes exposes a bug in the compiler (https://bugs.llvm.org/show_bug.cgi?id=45472), but other than that all existing OpenCL tests pass.

Default address space is a Clang concept that is generally used for an automatic storage. This is how embedded C defines it that has been used by Clang. OpenCL implementation has always used default for private and the decision to add private explicitly was made only about 2 years ago. OpenCL compiler infers address spaces for most of cases listed in the specification, however, there are cases that are not covered by the spec. There has not been any plan to forbid or remove the use of default addr space from the OpenCL implementation. And I don't think considering how long it was used historically it would be easy to do so.

> In SYCL mode we use default address space and apply conversion rules defined for generic address space, which is casting from generic to non-generic address space is allowed if memory is allocated in the same address space, otherwise behavior is undefined. In most cases, users don't need to worry about annotating pointers manually. They access memory through accessors which are annotated using template parameters, so template meta-programming will prevent invalid conversion. There is also an options to get a raw pointer and/or annotate a pointer using multi_ptr class, but in this case it's user's responsibility to make sure that conversion is valid. This might be required only in cases when compiler is not able to automatically optimize memory accesses to generic address space.
> 
> I was going to upstream CodeGen part with mapping "default address space" to LLVM address space in a separate patch. In the nutshell we would like to map the default address space to LLVM address space 4 for SPIR target (the same change is done for AMDGPU <https://github.com/intel/llvm/blob/sycl/clang/lib/Basic/Targets/AMDGPU.cpp#L41>). NOTE: variables with automatic storage duration in default address space (i.e. w/o explicit annotation) do not use address space map, but rather data layout string to set the LLVM address space for `alloca` instructions - the default value is 0, which is treated as private by SPIR-V translator.
> 
> I can work on updating this patch with CodeGen changes if you think both patches should be committed together.

I feel we will not make any progress without understanding and discussing your design first. If you envision large architectural changes we might need to make sure other stakeholders are informed about them.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80932