bader 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)); ---------------- 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? 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
