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

Wed Jan 13 10:25:26 PST 2021

bader added a comment.

In D89909#2448443 <https://reviews.llvm.org/D89909#2448443>, @Anastasia wrote:

> If you prefer to continue this route then you should just document your dialect sematic somewhere publicly accessible and avoid aliasing to OpenCL, or target address spaces. So it would become a proper language dialect implementation.

I've added a section to our internal design document about address space handling in SYCL mode: pull request <https://github.com/intel/llvm/pull/3027> (more readable version <https://github.com/intel/llvm/blob/249b241c4bf6514bbb2a0cbed0f4d7a0b0b3d93d/sycl/doc/CompilerAndRuntimeDesign.md#address-spaces-handling>). Please, take a look and let me know if it can be improved.

================
Comment at: clang/lib/Basic/Targets.cpp:577
-  case llvm::Triple::spir: {
-    if (Triple.getOS() != llvm::Triple::UnknownOS ||
-        Triple.getEnvironment() != llvm::Triple::UnknownEnvironment)
----------------
Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > We should not allow any arbitrary OS/Environment for SPIR.
> > @Anastasia, could you elaborate on that, please? Why can't we allow arbitrary values for these triple components?
> > 
> > The only reason I'm aware of is that LLVM-SPIRV-Translator was checking OS triple component and rejected LLVM module if value was not "unknow". Today it checks only "architecture' component and accepts only "spir" architecture. I personally think that even this restriction is artificial and LLVM module shouldn't be rejected based on the triple values, but it's a separate discussion.
> > 
> > I use additional environment component to choose between address space maps. We could update default address space in SPIRAddrSpaceMap directly, but IIRC, it caused some issues for OpenCL C++ mode, which uses Default address space instead of `opencl_private` in some cases.
> > 
> > Do you know if there is a better way to communicate the difference in Default address space to SPIRTargetInfo object?
> SPIR is not a real target but a virtual one and therefore it doesn't have different flavour for vendors or  and not run with any concrete OS. What good would it make to allow specifying `spir-apple-darwin10` or `spir-pc-win32`? What difference do we expect?
> 
> > Do you know if there is a better way to communicate the difference in Default address space to SPIRTargetInfo object?
> 
> If you only add the environment component to set the address spaces map then it doesn't feel like the right flow. In fact, your comments indicate that you aim to support the other targets too - how would this work for other targets that are not SPIR? And why are you specializing the map for SPIR but not other targets? 
> SPIR is not a real target but a virtual one and therefore it doesn't have different flavour for vendors or and not run with any concrete OS. What good would it make to allow specifying `spir-apple-darwin10` or `spir-pc-win32`? What difference do we expect?

I'm not sure I follow you. I don't see how OS or environment components are related to target architecture component. Additional information about execution environment allows compiler provide better results (e.g. configure include directories to system paths, set macro enabling code customization for particular OS, etc). I can imagine a situation when user would want write a code customized for different execution environments:
```
#if __linux__
...
#elseif _WIN32
...
#endif
```

and materialize "virtual LLVM IR" to x86 or ARM architecture.

> > Do you know if there is a better way to communicate the difference in Default address space to SPIRTargetInfo object?
> 
> If you only add the environment component to set the address spaces map then it doesn't feel like the right flow. In fact, your comments indicate that you aim to support the other targets too 

Not at all.

> And why are you specializing the map for SPIR but not other targets? 

Other targets correctly map Default AST address space to LLVM address space. The only other target specializing this map is AMDGPU and it selects the map based on architecture component of the target triple.

================
Comment at: clang/lib/Basic/Targets.cpp:577
-  case llvm::Triple::spir: {
-    if (Triple.getOS() != llvm::Triple::UnknownOS ||
-        Triple.getEnvironment() != llvm::Triple::UnknownEnvironment)
----------------
bader wrote:
> Anastasia wrote:
> > bader wrote:
> > > Anastasia wrote:
> > > > We should not allow any arbitrary OS/Environment for SPIR.
> > > @Anastasia, could you elaborate on that, please? Why can't we allow arbitrary values for these triple components?
> > > 
> > > The only reason I'm aware of is that LLVM-SPIRV-Translator was checking OS triple component and rejected LLVM module if value was not "unknow". Today it checks only "architecture' component and accepts only "spir" architecture. I personally think that even this restriction is artificial and LLVM module shouldn't be rejected based on the triple values, but it's a separate discussion.
> > > 
> > > I use additional environment component to choose between address space maps. We could update default address space in SPIRAddrSpaceMap directly, but IIRC, it caused some issues for OpenCL C++ mode, which uses Default address space instead of `opencl_private` in some cases.
> > > 
> > > Do you know if there is a better way to communicate the difference in Default address space to SPIRTargetInfo object?
> > SPIR is not a real target but a virtual one and therefore it doesn't have different flavour for vendors or  and not run with any concrete OS. What good would it make to allow specifying `spir-apple-darwin10` or `spir-pc-win32`? What difference do we expect?
> > 
> > > Do you know if there is a better way to communicate the difference in Default address space to SPIRTargetInfo object?
> > 
> > If you only add the environment component to set the address spaces map then it doesn't feel like the right flow. In fact, your comments indicate that you aim to support the other targets too - how would this work for other targets that are not SPIR? And why are you specializing the map for SPIR but not other targets? 
> > SPIR is not a real target but a virtual one and therefore it doesn't have different flavour for vendors or and not run with any concrete OS. What good would it make to allow specifying `spir-apple-darwin10` or `spir-pc-win32`? What difference do we expect?
> 
> I'm not sure I follow you. I don't see how OS or environment components are related to target architecture component. Additional information about execution environment allows compiler provide better results (e.g. configure include directories to system paths, set macro enabling code customization for particular OS, etc). I can imagine a situation when user would want write a code customized for different execution environments:
> ```
> #if __linux__
> ...
> #elseif _WIN32
> ...
> #endif
> ```
> 
> and materialize "virtual LLVM IR" to x86 or ARM architecture.
> 
> 
> > > Do you know if there is a better way to communicate the difference in Default address space to SPIRTargetInfo object?
> > 
> > If you only add the environment component to set the address spaces map then it doesn't feel like the right flow. In fact, your comments indicate that you aim to support the other targets too 
> 
> Not at all.
> 
> > And why are you specializing the map for SPIR but not other targets? 
> 
> Other targets correctly map Default AST address space to LLVM address space. The only other target specializing this map is AMDGPU and it selects the map based on architecture component of the target triple.
> SPIR is not a real target but a virtual one and therefore it doesn't have different flavour for vendors or  and not run with any concrete OS. What good would it make to allow specifying `spir-apple-darwin10` or `spir-pc-win32`? What difference do we expect?
> 
> > Do you know if there is a better way to communicate the difference in Default address space to SPIRTargetInfo object?
> 
> If you only add the environment component to set the address spaces map then it doesn't feel like the right flow. In fact, your comments indicate that you aim to support the other targets too - how would this work for other targets that are not SPIR? And why are you specializing the map for SPIR but not other targets? 

================
Comment at: clang/lib/Basic/Targets/SPIR.h:71
     LongWidth = LongAlign = 64;
-    AddrSpaceMap = &SPIRAddrSpaceMap;
+    if (Triple.getEnvironment() == llvm::Triple::SYCLDevice) {
+      AddrSpaceMap = &SYCLAddrSpaceMap;
----------------
Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > This deserves clarification - address space map reflect the physical address space of the target - how will upstream users get to physical address spaces of targets for SYCL?
> > The same way. The only difference for SYCL is that "Default" language address space is mapped to "4" LLVM AS, which is equivalent to "opencl_generic" language address space.
> If you are aiming at following the Embedded C semantic then this change is not legitimate. In Embedded C generic address space corresponds to no address space:
> 
> > When not specified otherwise, objects are allocated by default in a generic address space, which corresponds to the single address space of ISO/IEC 9899:1999
> 
> I.e. in LLVM project default address space is a flat address space of C/C++. You should either inherit it as is or use a different entry in the map... As a matter of fact, OpenCL generic address space is not the same as the generic address space of C and C++. It is only used for the pointers.
According to my understanding Embedded C semantics is applicable to LLVM address spaces and their mapping to SPIR(-V). SPIR-V translator treats "no address space" as private, rather than generic.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89909