[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling
Alexey Bader via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 9 12:24:46 PDT 2021
bader 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 ||
----------------
Anastasia wrote:
> 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)
> ```
> ?
> 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)
> ```
> ?
According to the comment above `isAddressSpaceSupersetOf` function definition.
```
/// Returns true if address space A is equal to or a superset of B.
```
`(A == LangAS::Default || B == LangAS::Default)` <- this change makes `Default` address space a superset of all address spaces including OpenCL, which we were trying to avoid with adding SYCL address spaces. Another problem with this code is that make `Default` a **sub-set** of named address spaces (like `sycl_local`), which is not right.
If I understand it correctly defining "isSupersSetOf" relation is enough for the rest of framework to enable conversions. Am I right?
================
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);
----------------
Anastasia wrote:
> 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.
> 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.
I'm not sure I understand your suggestion. Could you elaborate on "OpenCL mangling", please?
Let me clarify the problem this change addresses. The test case covering it is located in `clang/test/CodeGenSYCL/address-space-parameter-conversions.cpp` lines 86-91.
```
template <typename T>
void tmpl(T t) {}
int *NoAS;
__attribute__((opencl_private)) int *PRIV;
tmpl(PRIV);
// CHECK-DAG: [[PRIV_LOAD5:%[a-zA-Z0-9]+]] = load i32*, i32* addrspace(4)* [[PRIV]].ascast
// CHECK-DAG: call spir_func void [[PRIV_TMPL:@[a-zA-Z0-9_]+]](i32* [[PRIV_LOAD5]])
tmpl(NoAS);
// CHECK-DAG: [[NoAS_LOAD5:%[a-zA-Z0-9]+]] = load i32 addrspace(4)*, i32 addrspace(4)* addrspace(4)* [[NoAS]].ascast
// CHECK-DAG: call spir_func void [[GEN_TMPL:@[a-zA-Z0-9_]+]](i32 addrspace(4)* [[NoAS_LOAD5]])
```
Clang has separate code paths for mangling types w/ and w/o address space attributes (i.e. using `Default` address space).
Address space is not mangled if there is no AS attribute (`Default`) or if address space attribute is maps to `0` target address space. SPIR target maps `*_private` address space to `0`, which causes name conflict for the example above.
This change for SYCL compiler enables mangling for non-default address space attributes regardless of their mapping to target address space.
================
Comment at: clang/lib/Basic/Targets/AMDGPU.cpp:74
Local, // cuda_shared
+ Global, // sycl_global
+ Local, // sycl_local
----------------
Anastasia wrote:
> 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.
I can't find an example of how to do this.
CUDA address spaces use valid values and I wasn't able to find similar comments.
Where do you think we can put a comment?
================
Comment at: clang/lib/Basic/Targets/SPIR.h:36
0, // cuda_shared
+ 1, // sycl_global
+ 3, // sycl_local
----------------
Anastasia wrote:
> 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.
I've set 0 for SYCL values.
================
Comment at: clang/lib/Basic/Targets/SPIR.h:71
LongWidth = LongAlign = 64;
- AddrSpaceMap = &SPIRAddrSpaceMap;
+ AddrSpaceMap = Triple.getEnvironment() == llvm::Triple::SYCLDevice
+ ? &SPIRDefIsGenMap
----------------
Anastasia wrote:
> 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.
>
Good suggestion. I see that AMDGPU target does the same trick with overriding `ajust` method.
I also added back SPIR target assertions for unknow environment and OS components to clang/lib/Basic/Targets.cpp.
================
Comment at: clang/lib/Basic/Targets/SPIR.h:128
+ // }
+ return LangAS::sycl_global;
+ }
----------------
Anastasia wrote:
> This only makes sense for SYCL so let's add a guard.
> This only makes sense for SYCL so let's add a guard.
Unfortunately, I don't know what can be used as a guard here. TargetInfo doesn't provide access to language options.
Any suggestions?
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9954
unsigned getOpenCLKernelCallingConv() const override;
+ bool shouldEmitStaticExternCAliases() const override;
};
----------------
Anastasia wrote:
> Is this change related to address spaces?
I'll move this change to a separate patch.
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9984
+ !(CGM.getLangOpts().CUDA && CGM.getLangOpts().CUDAIsDevice) &&
+ "Address space agnostic languages only");
+ LangAS DefaultGlobalAS = getLangASFromTargetAS(
----------------
Anastasia wrote:
> 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++?
> 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++?
I think the assert says that OpenCL and CUDA are not address space agnostic languages.
Tagging @yaxunl as an author of the check I just borrowed from AMDGPU target.
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9985
+ "Address space agnostic languages only");
+ LangAS DefaultGlobalAS = getLangASFromTargetAS(
+ CGM.getContext().getTargetAddressSpace(LangAS::sycl_global));
----------------
Anastasia wrote:
> 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.
This code assigns target address space "global variables w/o address space attribute".
SYCL says it's "implementation defined" (from https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#subsec:commonAddressSpace):
> Namespace scope
> If the type is const, the address space the declaration is assigned to is implementation-defined. If the target of the SYCL backend can represent the generic address space, then the assigned address space must be compatible with the generic address space.
> Namespace scope non-const declarations cannot be used within a kernel, as restricted in Section 5.4. This means that non-const global variables cannot be accessed by any device kernel or code called by the device kernel.
I added clarification that SPIR target allocates global variables in global address space to https://reviews.llvm.org/D99488 (see line #248).
@rjmccall, mentioned in the mailing list discussion that this callbacks were developed for compiling C++ to AMDGPU target, so this not necessary designed only for SYCL, but it works for SYCL as well.
================
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
----------------
Anastasia wrote:
> Is this comment relevant?
> Is this comment relevant?
I think so. It's placed by the script and useful for test maintenance.
================
Comment at: clang/test/CodeGenSYCL/address-space-cond-op.cpp:39
+template <typename name, typename Func>
+__attribute__((sycl_kernel)) void kernel(Func kernelFunc) {
+ kernelFunc();
----------------
Anastasia wrote:
> This code doesn't seem to be relevant to the testing.
Probably not for this patch, but the next patch (https://reviews.llvm.org/D71016) makes it necessary to emit LLVM IR for this test. Is it okay to leave it as is?
================
Comment at: clang/test/CodeGenSYCL/address-space-of-returns.cpp:8
+const char *ret_char() {
+ return "N";
+}
----------------
Anastasia wrote:
> Are you testing address space segment binding here? Could you extend to other use cases too?
> Are you testing address space segment binding here? Could you extend to other use cases too?
This test checks that returned address space is generic and address space is correctly casted for character, array, reference and aggregate types.
Could you clarify how exactly do you want this test to be extended, please? I think I'm not getting it.
================
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
----------------
Anastasia wrote:
> What functionality in the patch does this test?
> What functionality in the patch does this test?
As I mentioned in the comment for clang/lib/AST/ItaniumMangle.cpp, there was a problem with instantiating templates for parameters which differs only by address space attribute. Lines 79-91 cover mangling changes.
================
Comment at: clang/test/CodeGenSYCL/address-space-parameter-conversions.cpp:99
+
+void usages2() {
+ __attribute__((opencl_private)) int *PRIV;
----------------
Anastasia wrote:
> The same functionality seems to be tested above.
Removed.
================
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) {
----------------
Anastasia wrote:
> Does this test anything from the patch?
> Does this test anything from the patch?
It's the same case as for `address-space-cond-op.cpp` test. This function defines an entry point for the device code. If it's not called, SYCL device compiler won't emit any LLVM IR output. I uploaded the final version of the test, which assumes that SYCL device compiler code generation part is in place. I would prefer to keep it instead of adding it back with my next patch. Is this okay?
================
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
+
----------------
Anastasia wrote:
> What do you test in this file?
> What do you test in this file?
This test was added to cover a bug in CodeGen with incompatible address spaces between pointers to a struct and its members.
As we replaced CodeGen changes with SPIR target callbacks I think it's not needed anymore. I removed this test.
BTW, the same applies to clang/test/CodeGenSYCL/address-space-cond-op.cpp. Should we remove it as well?
================
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
+
----------------
Anastasia wrote:
> Does this test also check conversions? Could this be merged with another conversions test if there is anything missing there?
> Does this test also check conversions? Could this be merged with another conversions test if there is anything missing there?
I renamed this to address-space-deduction.cpp and address-space-parameter-conversions.cpp to address-space-conversions.cpp.
================
Comment at: clang/test/SemaSYCL/address-space-parameter-conversions.cpp:25
+
+ bar(*NoAS);
+ bar2(*NoAS);
----------------
Anastasia wrote:
> Btw you don't seem to be testing the reverse conversion i.e. from named to `Default`.
> Btw you don't seem to be testing the reverse conversion i.e. from named to `Default`.
Don't `bar2(*GLOB);`, `bar2(*LOC);`, `bar2(*PRIV);` cover this conversion?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89909/new/
https://reviews.llvm.org/D89909
More information about the llvm-commits
mailing list