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
