[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

Wed Apr 21 03:19:08 PDT 2021

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