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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Just a few minor nits from me, but I'm mostly wondering: where are we at with this and are there still substantive changes required? (I looked through the comments, but there's a lot of back-and-forth since Oct and I'm not certain what's holding the patch back currently.)



================
Comment at: clang/include/clang/AST/Type.h:492
+            (isPtrSizeAddressSpace(B) || B == LangAS::Default)) ||
+           // Default is a superset of SYCL address spaces
+           (A == LangAS::Default &&
----------------



================
Comment at: clang/lib/AST/ItaniumMangle.cpp:2379
       unsigned TargetAS = Context.getASTContext().getTargetAddressSpace(AS);
-      if (TargetAS != 0)
+      if (TargetAS != 0 || (Context.getASTContext().getLangOpts().SYCLIsDevice))
         ASString = "AS" + llvm::utostr(TargetAS);
----------------



================
Comment at: clang/lib/Basic/Targets/SPIR.h:71-75
+    if (Triple.getEnvironment() == llvm::Triple::SYCLDevice) {
+      AddrSpaceMap = &SYCLAddrSpaceMap;
+    } else {
+      AddrSpaceMap = &SPIRAddrSpaceMap;
+    }
----------------



================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9997
+    if (auto ConstAS = CGM.getTarget().getConstantAddressSpace())
+      return ConstAS.getValue();
+  }
----------------
I called it out because it wasn't clear to me what the type actually was, but I see now that it's an `Optional<LangAS>`. I think it's useful to know that we're dealing with an optional, but it's not crucial.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89909