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: > 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: > ``` > > I looked though the code base and I see that explicit cast is used when raw pointer is casted to address space annotated type. I think we can always use explicit cast from `Default` to named address space instead of implicit cast. It might be even useful to avoid unintended implicit casts causing UB. @keryell, @Naghasan, what do you think if we update https://reviews.llvm.org/D99488 to disallow implicit casts from `Default` to named address space? I think it should be okay considering that current implementation doesn't use this type of casts (and I can't come up with a use case for it). Meanwhile I've added checks for that to clang/test/SemaSYCL/address-space-conversions.cpp. ================ 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: > 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. > 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. I would really love to avoid changes to the mangler (e.g. to be able to link binaries produced by different front-end like SYCL/OpenCL/CUDA), but I don't know the better way to address the issue Sorry, I don't get what do you suggest here. Could you clarify what exactly I should change, please? ================ Comment at: clang/lib/Basic/Targets/AMDGPU.cpp:74 Local, // cuda_shared + Global, // sycl_global + Local, // sycl_local ---------------- Anastasia wrote: > 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... > 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... I've set Generic value (i.e. `0`) and added a comment. ================ Comment at: clang/lib/Basic/Targets/SPIR.h:129 + TargetInfo::adjust(Opts); + setAddressSpaceMap(/*DefaultIsGeneric=*/Opts.SYCLIsDevice); + } ---------------- Anastasia wrote: > Anastasia wrote: > > Ok, do you still plan to add a `FIXME` as mentioned previously explaining > > why we need to reset the map here? > Btw I was just thinking of another alternative here. > > What do you think about just setting a value in `Default` AS entry then we > wouldn't need any extra map at all in this case? So something like: > > > ``` > AddrSpaceMap[LangAS::Default] = AddrSpaceMap[LangAS::opencl_generic]; > ``` > with a good explanation in `FIXME`? :) > > Btw I was just thinking of another alternative here. > > What do you think about just setting a value in `Default` AS entry then we > wouldn't need any extra map at all in this case? So something like: > > > ``` > AddrSpaceMap[LangAS::Default] = AddrSpaceMap[LangAS::opencl_generic]; > ``` > with a good explanation in `FIXME`? :) > I think this won't work because AddrSpaceMap is a constant. Regarding `FIXME`, could you point where it was previously mentioned, please? I think it might miss it. I can add a link to the SYCL spec - https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#subsec:genericAddressSpace. Will it be sufficient? ================ Comment at: clang/lib/CodeGen/TargetInfo.cpp:9985 + "Address space agnostic languages only"); + LangAS DefaultGlobalAS = getLangASFromTargetAS( + CGM.getContext().getTargetAddressSpace(LangAS::sycl_global)); ---------------- Anastasia wrote: > 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. > It seems like this change better fits to > `CodeGenModule::GetGlobalVarAddressSpace` that already contains very similar > logic? This was the original implementation (see https://reviews.llvm.org/D89909?id=299795), but @rjmccall suggested to use this callback instead. Both ways work for me, but the implementation proposed by John is easier to maintain. > 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. I've updated the code to use target address space. I also added an assertion for SYCL language mode, although I think SPIR doesn't support global variables in address spaces other than global or constant regardless of the language mode, so I think the logic is universal. ================ Comment at: clang/test/CodeGenSYCL/address-space-cond-op.cpp:8 + +// CHECK-LABEL: define {{.*}} @_Z3foobR1SS_( +// CHECK-NEXT: entry: ---------------- Anastasia wrote: > 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. > 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. Removed this test. ================ 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: > 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. Removed from all new tests. ================ Comment at: clang/test/CodeGenSYCL/address-space-of-returns.cpp:8 +const char *ret_char() { + return "N"; +} ---------------- Anastasia wrote: > 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? > 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? Right. Removed. ================ 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: > bader wrote: > > 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. > You are not testing the mangling though? > You are not testing the mangling though? Right, what we are checking here is that each instantiation emits a different function in LLVM IR. Exact mangling is not relevant to the check. ================ 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 ---------------- Anastasia wrote: > bader wrote: > > Anastasia wrote: > > > Is this change related? I thought we are not adding the environment > > > component after all... > > > > > > > > > Is this change related? I thought we are not adding the environment > > > component after all... > > > > While I was removing `-sycldevice` environment component from the patch, I > > noticed that one of the committed tests already uses it. > > https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGenSYCL/convergent.cpp#L2 > > > > Do you want to me to create a separate review request for this change? > I see, this looks trivial enough test clean up. You could just commit it > without any review perhaps? > I see, this looks trivial enough test clean up. You could just commit it > without any review perhaps? Done in https://github.com/llvm/llvm-project/commit/95c614afcd4de71d00a240d6a4a02c036c972ed0 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
