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: > > > > > > > > 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. > Okay. Just to make sure that all details are clear: > - We drop all SPIRTargetCodeGenInfo changes and getConstantAddressSpace for > SPIR target. > - SYCL language specific changes in CodeGen are not limited to CodeGenModule. > As you might notice CodeGenModule modifications lead to address space > changes, which are not expected by other part of the library. We need update > bitcast with addrspacecast instruction in a few places to. Basically it means > "take CodeGen library changes from the first patch". > - Most of the test cases removed during code review cover the cases mentioned > in the previous item and should be added back. > > Let me know if there any concerns with any of these items. > SYCL language specific changes in CodeGen are not limited to CodeGenModule. > As you might notice CodeGenModule modifications lead to address space > changes, which are not expected by other part of the library. We need update > bitcast with addrspacecast instruction in a few places to. Basically it means > "take CodeGen library changes from the first patch". I don't see how moving the same logic elsewhere would need extra address space casts? Your SPIR target changes have identical semantics to the diff I pasted above. 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
