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

[Anastasia Stulova via Phabricator via llvm-commits]

Anastasia added inline comments.


================
Comment at: clang/include/clang/AST/Type.h:493
+           // Default is a superset of SYCL address spaces.
+           (A == LangAS::Default &&
+            (B == LangAS::sycl_private || B == LangAS::sycl_local ||
----------------
Ok if you allow implicit conversions both ways then this condition should be extended to also contain all named address spaces in `A` and `Default` in `B`. But actually, could you simplify by checking that you have `Default` on either side, so something like 


```
(A == LangAS::Default || B == LangAS::Default)
```
?


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:2379
       unsigned TargetAS = Context.getASTContext().getTargetAddressSpace(AS);
-      if (TargetAS != 0)
+      if (TargetAS != 0 || Context.getASTContext().getLangOpts().SYCLIsDevice)
         ASString = "AS" + llvm::utostr(TargetAS);
----------------
Any reason not to use OpenCL mangling? If you do then you might be able to link against libraries compiled for OpenCL. Also you will get more stable naming i.e. it would not differ from target to target. 


================
Comment at: clang/lib/Basic/Targets/AMDGPU.cpp:74
     Local,    // cuda_shared
+    Global,   // sycl_global
+    Local,    // sycl_local
----------------
Would this map ever be used for SYCL? If not it would be better to add a comment about it and/or perhaps even just use dummy values.


================
Comment at: clang/lib/Basic/Targets/SPIR.h:36
     0, // cuda_shared
+    1, // sycl_global
+    3, // sycl_local
----------------
The same here. This map will never work for SYCL so let's just use dummy values like for CUDA and add a comment explaining this.


================
Comment at: clang/lib/Basic/Targets/SPIR.h:71
     LongWidth = LongAlign = 64;
-    AddrSpaceMap = &SPIRAddrSpaceMap;
+    AddrSpaceMap = Triple.getEnvironment() == llvm::Triple::SYCLDevice
+                       ? &SPIRDefIsGenMap
----------------
Ok so what I understand is that the only reason you need a separate map is that the semantics of `Default` is different for SYCL than for C/C++.

//i.e. in SYCL (i.e. inherited from CUDA) it is a virtual/placeholder address space that can point to any of the named address spaces while in embedded C it is a physical address space which is the same as automatic storage / flat address space.//

Since originally CUDA was not intended to compile for the same targets as C or C++ it hasn't been an issue as there was some sort of separation of concerns. But with wider adoption, it seems that this separation no longer exists and any target supporting CUDA style address space semantic together with C style would need to duplicate the address space map. This is not ideal especially considering the number of entities it contains and their constant growth. In reality, at least there won't be many such targets but if SYCL intends to support targets that OpenCL does then it is not uncommon. For example CPU targets can compile both OpenCL and C/C++...

If we adhere to the current architecture we should have C's `Default` and CUDA's `Default` as separate entries in the map. However, this might be quite an involved refactoring. Have you looked in this before? A less involved refactoring could be to add a special hook that would allow to alter the address space mapping only for Default address space. I am not sure whether this is something we could add to Targets, although it seems to be driven by the language semantic. Anyway we can't decide this in this review as it requires other community member involvement.

Considering that the workaround is relatively simple I think it would be reasonable to accept it as an initial step. However, I would prefer to simplify this patch further by removing the environment component for the time being and just conditionalizing the map on the language mode instead. You will probably need to reset the map in `TargetInfo:adjust` because this is where we have access to `LangOpts`.

https://clang.llvm.org/doxygen/Basic_2TargetInfo_8cpp_source.html#l00347

We already override lots of settings there and hopefully, this will only be needed temporarily. I would add a FIXME comment there explaining the design issue.

Then you won’t need to extend the triple component at least for now, until we find the right architectural solution for this. As I am not convinced this is a good approach. I would suggest to follow up on cfe-dev regarding this issue and how we should address this in the long term. Ideally, we should have started from this but I understand that you have some timing constraints.



================
Comment at: clang/lib/Basic/Targets/SPIR.h:123
+    // If we assign "opencl_constant" address space the following code becomes
+    // illegal, because it can't be cast to any other address space:
+    //
----------------
Should the following code be legal? Perhaps you can replace this with a spec reference instead?


================
Comment at: clang/lib/Basic/Targets/SPIR.h:128
+    //   }
+    return LangAS::sycl_global;
+  }
----------------
This only makes sense for SYCL so let's add a guard.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9954
   unsigned getOpenCLKernelCallingConv() const override;
+  bool shouldEmitStaticExternCAliases() const override;
 };
----------------
Is this change related to address spaces?


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9984
+         !(CGM.getLangOpts().CUDA && CGM.getLangOpts().CUDAIsDevice) &&
+         "Address space agnostic languages only");
+  LangAS DefaultGlobalAS = getLangASFromTargetAS(
----------------
I think this comment should be improved because OpenCL and CUDA are not really address space agnostic. Should this check also contain C or C++?


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9985
+         "Address space agnostic languages only");
+  LangAS DefaultGlobalAS = getLangASFromTargetAS(
+      CGM.getContext().getTargetAddressSpace(LangAS::sycl_global));
----------------
Since you are using SYCL address space you should probably guard this line by SYCL mode...  Btw the same seems to apply to the code below as it implements SYCL sematics?

Can you add spec references here too.

Also there seems to be nothing target specific in the code here as you are implementing what is specified by the language semantics. Should this not be moved to `GetGlobalVarAddressSpace` along with the other language handling?

I am not very familiar with this part of address space handling though. I would be more comfortable if @rjmccall could take a look too.


================
Comment at: clang/test/CodeGenSYCL/address-space-cond-op.cpp:1
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang_cc1 -x c++ -triple spir64-unknown-linux-sycldevice -disable-llvm-passes -fsycl -fsycl-is-device -emit-llvm %s -o - | FileCheck %s
----------------
Is this comment relevant?


================
Comment at: clang/test/CodeGenSYCL/address-space-cond-op.cpp:39
+template <typename name, typename Func>
+__attribute__((sycl_kernel)) void kernel(Func kernelFunc) {
+  kernelFunc();
----------------
This code doesn't seem to be relevant to the testing.


================
Comment at: clang/test/CodeGenSYCL/address-space-of-returns.cpp:8
+const char *ret_char() {
+  return "N";
+}
----------------
Are you testing address space segment binding here? Could you extend to other use cases too?


================
Comment at: clang/test/CodeGenSYCL/address-space-parameter-conversions.cpp:1
+// RUN: %clang_cc1 -triple spir64-unknown-linux-sycldevice -fsycl -fsycl-is-device -disable-llvm-passes -emit-llvm %s -o - | FileCheck %s
+void bar(int &Data) {}
----------------
How about adding explicit conversion here too and then renaming into something like `address-space-conversions.cpp` since the testing is not specific to a parameter.



================
Comment at: clang/test/CodeGenSYCL/address-space-parameter-conversions.cpp:80
+  // Ensure that we still get 3 different template instantiations.
+  tmpl(GLOB);
+  // CHECK-DAG: [[GLOB_LOAD4:%[a-zA-Z0-9]+]] = load i32 addrspace(1)*, i32 addrspace(1)* addrspace(4)* [[GLOB]].ascast
----------------
What functionality in the patch does this test?


================
Comment at: clang/test/CodeGenSYCL/address-space-parameter-conversions.cpp:99
+
+void usages2() {
+  __attribute__((opencl_private)) int *PRIV;
----------------
The same functionality seems to be tested above.


================
Comment at: clang/test/CodeGenSYCL/address-space-parameter-conversions.cpp:124
+
+template <typename name, typename Func>
+__attribute__((sycl_kernel)) void kernel_single_task(Func kernelFunc) {
----------------
Does this test anything from the patch?


================
Comment at: clang/test/CodeGenSYCL/address-spaces-struct.cpp:1
+// RUN: %clang_cc1 -triple spir64-unknown-linux-sycldevice -fsycl -fsycl-is-device -disable-llvm-passes -emit-llvm -x c++ %s -o - | FileCheck %s
+
----------------
What do you test in this file?


================
Comment at: clang/test/CodeGenSYCL/address-spaces.cpp:1
+// RUN: %clang_cc1 -triple spir64-unknown-linux-sycldevice -fsycl -fsycl-is-device -disable-llvm-passes -emit-llvm -x c++ %s -o - | FileCheck %s
+
----------------
Does this test also check conversions? Could this be merged with another conversions test if there is anything missing there?


================
Comment at: clang/test/SemaSYCL/address-space-parameter-conversions.cpp:1
+// RUN: %clang_cc1 -fsycl -fsycl-is-device -verify -fsyntax-only -x c++ %s
+
----------------
I think this should also be renamed to `address-space-conversions.cpp`

You shouldn't need to add `-x c++` as it is default for cpp files.

You should add testing of diagnostics for disallowed conversions too.


================
Comment at: clang/test/SemaSYCL/address-space-parameter-conversions.cpp:25
+
+  bar(*NoAS);
+  bar2(*NoAS);
----------------
Btw you don't seem to be testing the reverse conversion i.e. from named to `Default`.


================
Comment at: clang/test/SemaSYCL/address-space-parameter-conversions.cpp:47
+  (void)static_cast<void *>(GLOB);
+  // FIXME: determine if we can warn on the below conversions.
+  int *i = GLOB;
----------------
Why?


================
Comment at: clang/test/SemaSYCL/address-space-parameter-conversions.cpp:53
+
+  __generic int *IsGeneric; // expected-error{{unknown type name '__generic'}}
+  __private int *IsPrivate; // expected-error{{unknown type name '__private'}}
----------------
This doesn't belong to conversions technically.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89909