[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: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();
----------------
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.



================
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:
> 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.


https://reviews.llvm.org/D32248