[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling
Anastasia Stulova via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 12 07:56:52 PDT 2021
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 ||
----------------
bader wrote:
> 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?
> (A == LangAS::Default || B == LangAS::Default) <- this change makes Default address space a superset of all address spaces including OpenCL.
I see, yes this will break pretty much everything unless we guard by SYCL mode. But I don't think it is good to go this route though.
> Another problem with this code is that make Default a sub-set of named address spaces (like sycl_local), which is not right.
Well, if you need implicit conversions to work both ways as you have written in the documentation then you don't really have a true super-/subsets between the named address spaces and the default one. They appear to be equivalent.
```
SYCL mode enables both explicit and implicit conversion to/from the default address space from/to
the address space-attributed type.
```
So do you actually need something like this to work?
```
int * genptr = ...;
__private int * privptr = genptr:
```
================
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);
----------------
bader wrote:
> 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.
It's just that all language address spaces are mangled with the source spelling in Italium ABI right now, if you check the `else` statement. I don't think it is part of the official spec yet but it might be better to stick to the same pattern if possible.
================
Comment at: clang/lib/Basic/Targets/AMDGPU.cpp:74
Local, // cuda_shared
+ Global, // sycl_global
+ Local, // sycl_local
----------------
bader wrote:
> 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?
I think you could add a comment right here. I bet you can use any values i.e. `0`? They are not expected to be meaningful. It would be good if we could add an asset somewhere but perhaps it might be hard to find a good place...
================
Comment at: clang/lib/Basic/Targets/SPIR.h:46
+ 4, // Default
+ 1, // opencl_global
+ 3, // opencl_local
----------------
Ok let's set the OpenCL address spaces to `0` and add a comment that they can't be used with this map.
================
Comment at: clang/lib/Basic/Targets/SPIR.h:129
+ TargetInfo::adjust(Opts);
+ setAddressSpaceMap(/*DefaultIsGeneric=*/Opts.SYCLIsDevice);
+ }
----------------
Ok, do you still plan to add a `FIXME` as mentioned previously explaining why we need to reset the map here?
================
Comment at: clang/lib/Basic/Targets/SPIR.h:36
0, // cuda_shared
+ 1, // sycl_global
+ 3, // sycl_local
----------------
bader wrote:
> 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.
Great! Let's add a comment here that we use dummy values because the map can't be used for this language mode.
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9985
+ "Address space agnostic languages only");
+ LangAS DefaultGlobalAS = getLangASFromTargetAS(
+ CGM.getContext().getTargetAddressSpace(LangAS::sycl_global));
----------------
bader wrote:
> 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.
After all what objects are allowed to bind to non-default address space here is defined in SYCL spec even if the exact address spaces are not defined so it is not completely a target-specific behavior.
My understanding of the API you are extending (judging from its use) is that it allows you to extend the language sematics with some target-specific setup. I.e. you could add extra address spaces to C++ or OpenCL or any other language. But here you are setting the language address spaces instead that are mapped to the target at some point implicitly.
It seems like this change better fits to `CodeGenModule::GetGlobalVarAddressSpace` that already contains very similar logic?
Otherwise, it makes more sense to use target address spaces directly instead of SYCL language address spaces. But either way, we should guard it by SYCL mode somehow as we have not established this as a universal logic for SPIR.
================
Comment at: clang/test/CodeGenSYCL/address-space-cond-op.cpp:8
+
+// CHECK-LABEL: define {{.*}} @_Z3foobR1SS_(
+// CHECK-NEXT: entry:
----------------
I am trying to understand what specifically you are testing from the patch and whether you need this whole IR output to be checked?
It helps for reviewing and maintenance purposes to minimize the IR patterns as much as possible.
================
Comment at: clang/test/CodeGenSYCL/address-space-cond-op.cpp:39
+template <typename name, typename Func>
+__attribute__((sycl_kernel)) void kernel(Func kernelFunc) {
+ kernelFunc();
----------------
bader wrote:
> 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?
I would prefer if we minimize the amount of noise as much as possible. This patch has enough functionality so I don't think adding extra helps in any way.
The testing should generally encapsulate what is relevant. Otherwise, I don't know how to assess it without reviewing other patches too.
================
Comment at: clang/test/CodeGenSYCL/address-space-conversions.cpp:12
+// CHECK-DAG: define{{.*}} spir_func void @[[RAW_PTR2:[a-zA-Z0-9_]+]](i32 addrspace(4)* %
+void foo(__attribute__((opencl_local)) int *Data) {}
+// CHECK-DAG: define{{.*}} spir_func void [[LOC_PTR:@[a-zA-Z0-9_]+]](i32 addrspace(3)* %
----------------
Should you check that the function name has expected mangling?
================
Comment at: clang/test/CodeGenSYCL/address-space-of-returns.cpp:8
+const char *ret_char() {
+ return "N";
+}
----------------
bader wrote:
> 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.
It seems to me that you are just testing address space inference here similar to `address-space-deduction.cpp`? I don't see anything specific to a return type in either the test or your patch?
================
Comment at: clang/test/CodeGenSYCL/convergent.cpp:2
// RUN: %clang_cc1 -fsycl-is-device -emit-llvm -disable-llvm-passes \
-// RUN: -triple spir64-unknown-unknown-sycldevice -emit-llvm %s -o - | \
// RUN: FileCheck %s
----------------
Is this change related? I thought we are not adding the environment component after all...
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