[PATCH] D32248: CodeGen: Cast alloca to expected address space

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/CodeGen/CGDecl.cpp:1118
+        AddrTy->getElementType()->getPointerTo(ExpectedAddrSpace)),
+        address.getAlignment());
+  }
----------------
This is a lot of work to be doing in a pretty common routine for the benefit of one unusual target.  Maybe there's some way to fast-path it, like you can cache the AST address space of the stack on the CGM (assuming it's always got its own AST address space, I guess) and just check whether Ty.getAddressSpace() is different from that.

Also, I feel like general routines in IRGen should never be calling CreateAddrSpaceCast; they should always be calling getTargetCodeGenInfo().performAddrSpaceCast instead, just in case the target has some more specific idea about how the conversion should be done.  I know we're not consistent about that right now, but it's something to aim for.

Also, I agree with Tony Tye: if it's an intended feature that you can declare locals in the constant address space, you need to figure out what the allocation semantics are for that and emit the appropriate code rather than just hand-waving it here after the fact.


================
Comment at: lib/CodeGen/CodeGenTypes.h:200
+  /// Get the LLVM pointer type of a variable.
+  llvm::PointerType *getVariableType(VarDecl D);
+
----------------
yaxunl wrote:
> t-tye wrote:
> > Should the name reflect that the type returned is not the variable type, but a pointer to the variable type? For example, getVariablePointerType or getPointerToVariableType.
> I think getPointerToVariableType is better. I will change it.
The only time you use this function is to immediately call getAddressSpace on it.  Seems to me you should just call getTargetAddressSpace(D.getType()).


https://reviews.llvm.org/D32248