[PATCH] D32248: CodeGen: Cast alloca to expected address space

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: lib/CodeGen/CodeGenTypes.cpp:95
 
-
 /// isRecordLayoutComplete - Return true if the specified type is already
----------------
Why this change?


================
Comment at: lib/CodeGen/TargetInfo.cpp:411
+    CodeGen::CodeGenFunction &CGF, llvm::Value *Src, unsigned SrcAddr,
+    unsigned DestAddr, llvm::Type *DestTy, bool isNonNull) const {
   // Since target may map different address spaces in AST to the same address
----------------
Seems like some parameters are unused.


================
Comment at: lib/Sema/SemaDecl.cpp:7206
+    assert(T.getAddressSpace() != LangAS::opencl_constant);
+    if (T.getAddressSpace() == LangAS::opencl_global) {
+      Diag(NewVD->getLocation(), diag::err_opencl_function_variable)
----------------
I think it was nicer that all OpenCL related changes were grouped in one spot under the same if statement. Even though it isn't quite done everywhere. But this change makes them even more scattered.


================
Comment at: test/CodeGenCXX/amdgcn-automatic-variable.cpp:25
+  // CHECK: store i32 2, i32* %[[r1]]
+  int lv1;
+  lv1 = 1;
----------------
yaxunl wrote:
> yaxunl wrote:
> > Anastasia wrote:
> > > I am wondering if all these different test cases are really needed. Are we testing any different program paths from the patch?
> > > 
> > > Also would it make sense to add a test case with an object in an AS different to 0 (i.e. with `__attribute__((address_space(n)))`)
> > I think at least I should cover the typical use cases of auto var.
> > 
> > I will add a test for __attribute__((address_space(n))))
> Sorry. I just checked that C++ does not allow __attribute__((address_space(n))) on automatic var.
I thought address space was undocumented extension. So essentially you can't have a cast to any address space but a default here? Would it work for the C instead to add `attribute((address_space(n)))`?


================
Comment at: test/SemaOpenCL/storageclass-cl20.cl:16
+  global int L3;                              // expected-error{{function scope variable cannot be declared in global address space}}
+  __attribute__((address_space(100))) int L4; // expected-error{{automatic variable qualified with an invalid address space}}
 
----------------
I am guessing generic should be rejected here as well... although it should only apply to pointers. But it seems we don't handle this properly.


https://reviews.llvm.org/D32248