[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:1115
+  assert(T.getAddressSpace() == LangAS::Default ||
+         T.getQualifiers().hasTargetSpecificAddressSpace());
+  auto Addr = getTargetHooks().performAddrSpaceCast(*this,
----------------
yaxunl wrote:
> t-tye wrote:
> > Should allowing specifying an address space on a function local automatic variable in OpenCL be allowed? It seems generating LLVM IR that allocates the variable in the alloca address space then address space casting the pointer to some other address space is not what such a language feature is requesting.
> > 
> > It seems it is really requesting that the variable is allocated in a different address space. That could be indicated by putting a different address space on the alloca itself, but the builder only allows an alloca to use the default alloca address space. No target supports this ability.
> > 
> > So maybe the best solution is to make OpenCL be the same as C and not allow an address space qualifier on function scope automatic variables.
> > 
> > If that is done then this assert will only allow the language default address space.
> Agree. I think that is why C++ forbids `__attribute__((address_space(n))` on automatic variables. I will make it apply to all languages.
Yeah, the embedded C specification (ISO/IEC TR 18037) says "an address space name cannot be used to qualify an object that has automatic storage duration", so this restriction should be applied regardless of language mode.  We can deal with the possibility of auto variables in other address spaces when someone wants to add that capability.

(OpenCL's __constant is an acceptable exception because it implicitly makes the variable static.)




================
Comment at: lib/CodeGen/CGDecl.cpp:1119
+      T.getAddressSpace(), address.getPointer()->getType()->
+      getPointerElementType()->getPointerTo(getContext().getTargetAddressSpace(
+          T.getAddressSpace())), true);
----------------
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);


================
Comment at: lib/CodeGen/TargetInfo.cpp:7294
       llvm::PointerType *T, QualType QT) const override;
+
 };
----------------
Spurious newline?


https://reviews.llvm.org/D32248