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();
----------------
Anastasia wrote:
> rjmccall wrote:
> > 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.
> I belive we already do similar logic for stack variables with `__local` AS. 
> Because they are emmited as static globals. There should be no contradictions 
> with spec to do the same for `__constant `AS objects. They are essentially 
> intended to be located in a "special" memory sections and not on a stack. 
> Also constant AS local variable can only be declared in the kernel functions.
I created another patch for this issue: https://reviews.llvm.org/D32977


https://reviews.llvm.org/D32248



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to