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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
