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: > > > > > > > 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? 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
