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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.


================
Comment at: lib/CodeGen/CGDecl.cpp:1119
+      T.getAddressSpace(), address.getPointer()->getType()->
+      getPointerElementType()->getPointerTo(getContext().getTargetAddressSpace(
+          T.getAddressSpace())), true);
----------------
rjmccall wrote:
> Hmm.  I would prefer if we could avoid doing this in the fast path.  Suggestion:
> 
>   - Please add a variable (ASTAllocaAddressSpace) to CodeGenTypeCache that stores the AST address space of the stack.  You can add a function to CodeGenTargetInfo to initialize this; in most targets, it will just return 0, but your target can make it ~0U or something, anything works as long as it doesn't incorrectly match an actual target address space.  (But you should really consider adding an actual target-specific address space for the stack!  It'll probably be useful in builtin code or something eventually.)
> 
>   - Please add an assert that T.getAddressSpace() == 0, and then make this call conditional on whether ASTAllocaAddressSpace is non-zero.
> 
> You can just write address.getElementType() instead of address.getPointer()->getType()->getPointerElementType().
> 
> Are you intentionally leaving emission.Addr as the unconverted pointer?  That will probably mess everything else downstream that uses 'emission', and the only reason you're not seeing that is that OpenCL probably doesn't actually have anything downstream that uses 'emission' — destructors or whatever else.
> 
> Please add an inline comment like
>    , /*non-null*/ true);
Sorry I missed this comment. I will do the changes.

About emission.Addr, I leave the unconverted pointer intentionally. I need this to work for both C++ and OpenCL. I checked the usage of emission.Addr and they seem to be only load or store, so I think it is safe to use the unconverted pointer. Now you mention destructor. I think I need to change that and I am going to add a lit test also.


https://reviews.llvm.org/D32248