[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling
Aaron Ballman via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 3 10:20:03 PST 2021
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
More information about the llvm-commits
mailing list