bader added inline comments.
================ 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: > > > 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? > For now I am just trying to understand why you are not adopting similar > mangling scheme as for other language address spaces since it gives more > stable mangling irrespective from the target compiled for. > > If you plan to link libraries from other frontends i.e. OpenCL or CUDA the > mangling you use is different from what they produce. Just have a look at the > line 2470 that explains OpenCL mangling or line 2494 explaining CUDA > mangling. FYI similar scheme applies to other language address spaces, so the > `AS<num>` was only really used for the address spaces that have no source > spelling i.e. no language semantics. > For now I am just trying to understand why you are not adopting similar > mangling scheme as for other language address spaces since it gives more > stable mangling irrespective from the target compiled for. According to my understanding this code is used for other language spaces. For instance, per comments at lines 2455-2457 it's used for OpenCL and CUDA address spaces. Do you mean some other mangling scheme? > If you plan to link libraries from other frontends i.e. OpenCL or CUDA the > mangling you use is different from what they produce. SYCL standard doesn't have such functionality. OpenCL C functions are not mangled (only built-ins), so there should be no problem to link with OpenCL C libraries. I know that mangling difference causes some problems for SYCL built-ins implementations on CUDA, but I don't know all the details. @Naghasan knows about these. @Naghasan, do you have suggestions to fix the problems caused by mangling? > Just have a look at the line 2470 that explains OpenCL mangling or line 2494 > explaining CUDA mangling. FYI similar scheme applies to other language > address spaces, so the `AS<num>` was only really used for the address spaces > that have no source spelling i.e. no language semantics. Is this statement correct? https://godbolt.org/z/n7The38dz - `AS<num>` is used for OpenCL address spaces mangling when it's compiled for `spir` target. ================ Comment at: clang/lib/Basic/Targets/SPIR.h:129 + TargetInfo::adjust(Opts); + setAddressSpaceMap(/*DefaultIsGeneric=*/Opts.SYCLIsDevice); + } ---------------- Anastasia wrote: > bader wrote: > > 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? > I mean the FIXME is about the issue with Clang design: > https://reviews.llvm.org/D89909#inline-941733 > > Basically, just explain why we need to reset the map and that it is only > needed for `Default` address space. Added FIXME comment. ================ 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: > > > 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. > > This was the original implementation (see > > https://reviews.llvm.org/D89909?id=299795), but @rjmccall suggested to use > > this callback instead. > > Did you mean to link some particular conversation? Currently, it resets to > the top of the review. > > > Both ways work for me, but the implementation proposed by John is easier > > to maintain. > > I can't see why the same code would be harder to maintain in the caller. If > anything it should reduce the maintenance because the same logic won't need > to be implemented by every target. > > > 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. > > Asserts don't guard this logic to be applied universally. And since the IR > was generated like this for about 10 years I don't feel comfortable about > just changing it silently. > > To my memory SPIR spec never put restrictions to the address spaces. It only > described the generation for OpenCL C. So if you compile from C you would > have everything in the default address space. And even OpenCL rules doesn't > seem to be quite accurate in your patch as in OpenCL C globals can be in > `__global`, `__constant` or `__local`. However, the SPIR spec was > discontinued quite a while ago and the implementation of SPIR has evolved so > I am not sure how relevant the spec is now. > > Personally, I feel the behavior you are implementing is governed by the > language soI think it is more logical to encapsulate it to avoid interfering > with other language modes. > > > This was the original implementation (see > > https://reviews.llvm.org/D89909?id=299795), but @rjmccall suggested to use > > this callback instead. > > Did you mean to link some particular conversation? Currently, it resets to > the top of the review. I pointed to the patch version implementing address space deduction in `CodeGenModule::GetGlobalVarAddressSpace`. Conversion pointer is RFC in the mailing list: http://clang-developers.42468.n3.nabble.com/RFC-Re-use-OpenCL-address-space-attributes-for-SYCL-tp4068754p4069039.html > > Both ways work for me, but the implementation proposed by John is easier > > to maintain. > > I can't see why the same code would be harder to maintain in the caller. If > anything it should reduce the maintenance because the same logic won't need > to be implemented by every target. > > > 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. > > Asserts don't guard this logic to be applied universally. And since the IR > was generated like this for about 10 years I don't feel comfortable about > just changing it silently. > > To my memory SPIR spec never put restrictions to the address spaces. It only > described the generation for OpenCL C. So if you compile from C you would > have everything in the default address space. And even OpenCL rules doesn't > seem to be quite accurate in your patch as in OpenCL C globals can be in > `__global`, `__constant` or `__local`. However, the SPIR spec was > discontinued quite a while ago and the implementation of SPIR has evolved so > I am not sure how relevant the spec is now. > > Personally, I feel the behavior you are implementing is governed by the > language soI think it is more logical to encapsulate it to avoid interfering > with other language modes. > Added early exist for non-SYCL modes. 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