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