[PATCH] D111566: [SYCL] Fix function pointer address space

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:11496
+                   .getProgramAddressSpace()
+             : getTargetAddressSpace(T.getQualifiers());
+}
----------------
Oh, I'm sorry I missed this.  Parsing the data layout string into an `llvm::DataLayout` is definitely not an okay thing to be doing here.  The IRGen version of this had a cached `DataLayout` object which it queried, which was okay, but this function is used too tightly to be doing that much redundant work.

We could just cache a `DataLayout` in the `clang::TargetInfo`, but I think we've been trying to avoid that as a layering violation.  Instead, `TargetInfo` should just have a `getProgramAddressSpace` method or something like that, and the targets with non-default address spaces for code should set that manually.


================
Comment at: clang/lib/AST/ASTContext.cpp:11497
+                   .getProgramAddressSpace()
+             : getTargetAddressSpace(T.getQualifiers());
+}
----------------
eandrews wrote:
> rjmccall wrote:
> > If a function type has an address space qualifier, we should prefer that, right?  Or is that impossible by construction?
> I thought you could only use address space qualifiers for variables. I am not sure though. 
> 
> This patch retains existing behavior for pointers.  The existing code (deleted in CodeGenTypes.cpp in his patch) doesn't take qualifiers into consideration. 
Ah, yes, I see.  Alright.


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

https://reviews.llvm.org/D111566