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