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

Tue Dec 8 07:15:25 PST 2020

Anastasia added a comment.

In D89909#2423750 <https://reviews.llvm.org/D89909#2423750>, @bader wrote:

>> It was mentioned that the changes in the type system with address spaces is undesirable for SYCL because you indend to parse existing C++ code as-is. This contradicts the intended semantic of address spaces where the whole point of it is to modify the standard types and therefore a compilation of C++ with the standard semantic is only guaranteed when the attribute is not used at all.
>
> Right, but I don't think it's related to the address space attributes. It was mentioned in the context of re-using OpenCL *mode* for SYCL device compilation, which modifies types which does use address space attribute explicitly. "Existing C++ code" doesn't use address space attributes and our solution does differentiate explicitly annotated type. The difference with OpenCL mode is that SYCL doesn't change types by modifying "default" address space attribute and allows conversion from/to "default" address space. As I mentioned in RFC, according to my understanding this patch doesn't contradict Embedded C requirements regarding address space attribute usage. I think the spec allows an implementation to define conversion rules between address spaces and doesn't imply type change based on the declaration scope - it's OpenCL specific behavior.

Ok, if you plan to follow the Embedded C semantic (which your current patch confirms) then you should just reuse the existing target address spaces and extend the implementation with the ability to specify the relation among address spaces. The patch that you have mentioned earlier https://reviews.llvm.org/D62574 is providing this logic already and it looks good aside from testing. I would suggest to discuss with @ebevhan the timeline for committing it as the testing could be done using SYCL implementation. Alternatively, you could consider reusing the relevant parts of the patch if @ebevhan has no objections to that.

>> Perhaps to unblock your work it would be good to have a summary of what functionality you require from the address space attribute and what needs to work differently. Then at least it would be more clear if the attribute is a good fit here and if so what difference in its semantic will be brought by SYCL customizations. Whichever route we decide to go ahead, the documentation of intended language semantic should be added somewhere publicly accessible to facilitate adequate code review and maintenance because as far as I am aware it is not documented as part of the SYCL spec or any other documentation resource.
>
> The only difference directly related to clang's "opencl_*" address space attributes is that SYCL allows conversion between types with OpenCL address space attributes and "default" address space, but it's an implementation detail. The main sematic difference is that SYCL doesn't change "default" address space and it's "visible" with template metaprogramming or type deduction at compile time. It looks like the best place to describe this difference is a separate document similar to https://clang.llvm.org/docs/OpenCLSupport.html, right?

Yes, this should be good enough. I imagine you won't need a lot to document if you reuse target address space and an extension for targets to configure address space relations mentioned above (which we should document in the common C/C++ extensions btw).

================
Comment at: clang/lib/Basic/Targets.cpp:577
-  case llvm::Triple::spir: {
-    if (Triple.getOS() != llvm::Triple::UnknownOS ||
-        Triple.getEnvironment() != llvm::Triple::UnknownEnvironment)
----------------
We should not allow any arbitrary OS/Environment for SPIR.

================
Comment at: clang/lib/Basic/Targets/SPIR.h:71
     LongWidth = LongAlign = 64;
-    AddrSpaceMap = &SPIRAddrSpaceMap;
+    if (Triple.getEnvironment() == llvm::Triple::SYCLDevice) {
+      AddrSpaceMap = &SYCLAddrSpaceMap;
----------------
This deserves clarification - address space map reflect the physical address space of the target - how will upstream users get to physical address spaces of targets for SYCL?

================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:10019
+
+LangAS SPIRTargetCodeGenInfo::getGlobalVarAddressSpace(CodeGenModule &CGM,
+                                                       const VarDecl *D) const {
----------------
You should document this functionality. I don't think the dialects will need to call this function.

================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:10035
+  if (CGM.isTypeConstant(D->getType(), false)) {
+    if (auto ConstAS = CGM.getTarget().getConstantAddressSpace())
+      return ConstAS.getValue();
----------------
You should not have constant address space in SYCL!

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89909/new/

https://reviews.llvm.org/D89909