Anastasia added inline comments.
================ 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: > > bader wrote: > > > 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. > > > 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 > > > > > > > This looks actually very neat and I can't see that anyone had any concerns > > about this. > > > > I think John's comment on RFC is to point out that there are also Target > > hooks available should you need to override the language semantics but > > there is no statement that you should prefer it instead of implementing the > > language rules. I think the language semantics should take precedence. > > > > > Added early exist for non-SYCL modes. > > > > > > To improve the understanding I would prefer if you guard the logic with if > > statement and return the original address space as default right at the end: > > > > ``` > > > > if (CGM.getLangOpts().SYCLIsDevice) { > > // do what you need for SYCL > > } > > // default case - just return original address space > > return AddrSpace; > > ``` > > > 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 > > > > > > > This looks actually very neat and I can't see that anyone had any concerns > > about this. > > > > I think John's comment on RFC is to point out that there are also Target > > hooks available should you need to override the language semantics but > > there is no statement that you should prefer it instead of implementing the > > language rules. I think the language semantics should take precedence. > > > > Do I understand it correctly that you suggest replacing Target hooks with the > CodeGen library changes from [[ https://reviews.llvm.org/D89909?id=299795 | > the first version ]] of the patch? > @rjmccall, are you okay with that? Yes, that's right. I suggest lifting this logic into `CodeGenModule::GetGlobalVarAddressSpace`, so something like ``` diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp --- a/clang/lib/CodeGen/CodeGenModule.cpp +++ b/clang/lib/CodeGen/CodeGenModule.cpp @@ -3849,6 +3849,12 @@ return AddrSpace; } + if (LangOpts.SYCLIsDevice) { + if (!D || D->getType().getAddressSpace() == LangAS::Default) { + return LangAS::opencl_global; + } + } + if (LangOpts.CUDA && LangOpts.CUDAIsDevice) { if (D && D->hasAttr<CUDAConstantAttr>()) return LangAS::cuda_constant; @@ -3874,8 +3880,19 @@ // OpenCL v1.2 s6.5.3: a string literal is in the constant address space. if (LangOpts.OpenCL) return LangAS::opencl_constant; + + // If we keep a literal string in constant address space, the following code + // becomes illegal: + // + // const char *getLiteral() n{ + // return "AB"; + // } + if (LangOpts.SYCLIsDevice) + return LangAS::opencl_private; + if (auto AS = getTarget().getConstantAddressSpace()) return AS.getValue(); + return LangAS::Default; } ``` from your original patchset. 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
