[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

Wed Feb 3 10:28:50 PST 2021

Anastasia added a comment.

> Sorry, I forgot to mentioned that this change was also discussed in the mailing list: http://clang-developers.42468.n3.nabble.com/RFC-Re-use-OpenCL-address-space-attributes-for-SYCL-td4068754.html.
> There are no objections from the community.

I remember asking similar questions regarding the SYCL address space map on that thread, and I don't think I could get more than what we are discussing here. I generally don't think that RFC ended with any conclusions as a result this patch contained different approach to what I have suggested at the end.

> SPIR format evolved from SPIR-1.2 to SPIR-V, which is not based on LLVM-based, but AFAIK the goal of SPIR target in clang tool has not changed and it is still to produce binary format for Khronos APIs (even though part of the toolchain is hosted outside of LLVM project).

I would like to highlight that SPIR was completely replaced by SPIR-V. I don't really see it as evolution. There were several attempts to add SPIR-V into LLVM and Clang - both of us were involved in some of those.

As of now the comments in the source code still says:

  llvm/include/llvm/ADT/Triple.h:    spir,           // SPIR: standard portable IR for OpenCL 32-bit version
  llvm/include/llvm/ADT/Triple.h:    spir64,         // SPIR: standard portable IR for OpenCL 64-bit version

> It's great if there are other uses of SPIR target in LLVM project. Do you know where I can learn more about this?

I presume it is used for what SPIR was originally intended - a de-facto portable format?  As a matter of fact not all OpenCL vendors support SPIR-V and we as a community won't and can't force this. If you want to learn more about use cases I would ask the community, cfe-dev could be a good start.

> I puzzled by existing mapping: Aren't Default and SPIR-V Function (or OpenCL private) semantically different address spaces?

Yes they are. OpenCL private is just the most common case this is why mapping them to the same value often works.

> Note that LLVM IR objects are not allocated neither in Default nor in Generic address spaces as described by John in this message.

`Default` can be used to allocate objects even if address spaces for `alloca` can be customized. There are other objects that are used with `Default` - arguments or return types. It is just so inherent in the parser. Most of objects use `Default` at the AST level even if not all of this ends up in the IR.

> Could you provide an example where mapping Default to SPIR-V generic might cause the problem, please? I have a problem with imagining such case, but I can be biased.

Sorry I don't think I have enough time. But in general I think you should not base the design decision solely on the examples as both C and C++ are complex enough to overlook various corner cases. Your design should make sense conceptually and then the tooling will be correct by construction and easy to reason about.

> FWIW, real GPU targets map Default to Generic and as far as I concerned it doesn't cause problems.

How do you prove this though?

> Right, but we use SPIR target and it uses 4.

I guess I still don't get your architecture, but you could also look at adding a pass that remaps the address spaces or extend the pass to work with different default address spaces?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89909