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: > > > > > > > > > > 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. > > > Probably I misunderstand your proposal. Do you suggest moving > `SPIRTargetCodeGenInfo::getGlobalVarAddressSpace` to > `CodeGenModule::GetGlobalVarAddressSpace`, but keep the rest of > `SPIRTargetCodeGenInfo` and `SPIRTargetInfo` changes? Yes, indeed. 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
