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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to