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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 14 inline comments as done.
yaxunl added inline comments.


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


================
Comment at: lib/CodeGen/TargetInfo.cpp:7294
       llvm::PointerType *T, QualType QT) const override;
+
 };
----------------
rjmccall wrote:
> Spurious newline?
will remove it.


================
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)
----------------
Anastasia wrote:
> 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.
Will move them down.


================
Comment at: test/CodeGenCXX/amdgcn-automatic-variable.cpp:25
+  // CHECK: store i32 2, i32* %[[r1]]
+  int lv1;
+  lv1 = 1;
----------------
Anastasia wrote:
> 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)))`?
As John pointed out, this due to the embedded C specification (ISO/IEC TR 18037). Clang applies it to all languages except OpenCL.


================
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}}
 
----------------
Anastasia wrote:
> 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.
will add a line for generic.


https://reviews.llvm.org/D32248