rjmccall added inline comments.
================
Comment at: lib/CodeGen/CGDecl.cpp:1105-1119
+ // Alloca always returns a pointer in alloca address space, which may
+ // be different from the type defined by the language. For example,
+ // in C++ the auto variables are in the default address space. Therefore
+ // cast alloca to the expected address space when necessary.
+ auto Addr = address.getPointer();
+ auto AddrTy = cast<llvm::PointerType>(Addr->getType());
+ auto ExpectedAddrSpace =
CGM.getTypes().getVariableType(D)->getAddressSpace();
----------------
yaxunl wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > Anastasia wrote:
> > > > yaxunl wrote:
> > > > > yaxunl wrote:
> > > > > > t-tye wrote:
> > > > > > > Is any assert done to ensure that it is legal to address space
> > > > > > > cast from variable address space to expected address space?
> > > > > > > Presumably the language, by definition, will only be causing
> > > > > > > legal casts. For example from alloca address space to generic
> > > > > > > (which includes the alloca address space).
> > > > > > >
> > > > > > > For OpenCL, can you explain how the local variable can have the
> > > > > > > constant address space and use an alloca for allocation? Wouldn't
> > > > > > > a constant address space mean it was static and so should not be
> > > > > > > using alloca? And if it is using an alloca, how can it then be
> > > > > > > accessed as if it was in constant address space?
> > > > > > If the auto var has address space qualifier specified through
> > > > > > `__attribute__((address_space(n)))`, there is not much we can check
> > > > > > in clang since it is target dependent. We will just emit address
> > > > > > space cast when necessary and let the backend check the validity of
> > > > > > the address space cast.
> > > > > >
> > > > > > Otherwise, for OpenCL, we can assert the expected address space is
> > > > > > default (for OpenCL default address space in AST represents private
> > > > > > address space in source language) or constant. For other languages
> > > > > > we can assert the expected address space qualifier is default (no
> > > > > > address space qualifier). It is not convenient to further check
> > > > > > whether the emitted LLVM address space cast instruction is valid
> > > > > > since it requires target specific information, therefore such check
> > > > > > is better deferred to the backend.
> > > > > >
> > > > > > For OpenCL, currently automatic variable in constant address space
> > > > > > is emitted in private address space. For example, currently Clang
> > > > > > does not diagnose the following code
> > > > > >
> > > > > > ```
> > > > > > void f(global int* a) {
> > > > > > global int* constant p = a;
> > > > > > }
> > > > > >
> > > > > > ```
> > > > > > Instead, it emits alloca for p, essentially treats it as `global
> > > > > > int* const p`. This seems to be a bug to me (or maybe we can call
> > > > > > it a feature? since there seems no better way to translate this to
> > > > > > LLVM IR, or simply diagnose this as an error). However, this is
> > > > > > better addressed by another patch.
> > > > >
> > > > > Hi Anastasia,
> > > > >
> > > > > Any comments about the automatic variable in constant address space?
> > > > > Thanks.
> > > > From the spec s6.5.3 it feels like we should follow the same
> > > > implementation path in Clang for constant AS inside kernel function as
> > > > local AS. Because constant AS objects are essentially global objects.
> > > >
> > > > Although, we didn't have any issues up to now because const just does
> > > > the trick of optimising the objects out eventually. I am not clear if
> > > > this creates any issue now with your allocation change. It feels though
> > > > that it should probably work fine just as is?
> > > If these __constant locals are required to be const (or are implicitly
> > > const?) and have constant initializers, it seems to me the implementation
> > > obviously intended by the spec is that you allocate them statically in
> > > the constant address space. It's likely that just implicitly making the
> > > variable static in Sema, the same way you make it implicitly const, will
> > > make IRGen and various other parts of the compiler just do the right
> > > thing.
> > My patch does not change the current behaviour of Clang regarding
> > function-scope variable in constant address space. Basically there is no
> > issue if there is no address taken. However, if there is address taken,
> > there will be assertion due to casting private pointer to constant address
> > space. I think probably it is better to emit function-scope variable in
> > constant address space as global variable in constant address space instead
> > of alloca, as John suggested.
> >
> I agree function-scope variable in constant address space should be emitted
> as global variable in constant address space instead of alloca. However, in
> OpenCL 1.2 section 6.8, "The static storage-class specifier can only be used
> for non-kernel functions and global variables declared in program scope."
> Currently Clang diagnoses function-scope variables with static storage class
> for OpenCL 1.2. Therefore we cannot make function-scope variable in constant
> address space have static storage class. However, I think we can still emit
> function-scope variable in constant address space as global variable in
> CodeGen.
There's precedent for treating something as implicitly static in the AST.
thread_local variables in C++ are considered implicitly static.
You'll need to change VarDecl::hasLocalStorage() and isStaticLocal(), and you
should look around for other uses of getTSCSpec() that look like they're
implementing the same logic.
https://reviews.llvm.org/D32248
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
