[PATCH] D21567: [OpenCL] Generate struct type for sampler_t and function call for the initializer

[Yaxun Liu via cfe-commits]

yaxunl marked 9 inline comments as done.

================
Comment at: lib/CodeGen/CGOpenCLRuntime.cpp:84
@@ +83,3 @@
+    SamplerTy = llvm::PointerType::get(llvm::StructType::create(
+      CGM.getLLVMContext(), "__opencl_sampler_t"),
+      CGM.getContext().getTargetAddressSpace(
----------------
Anastasia wrote:
> Related to this thread: http://lists.llvm.org/pipermail/cfe-dev/2016-July/050126.html.
> 
> I am guessing leaving the old naming scheme should be fine.
I have some more comments about that. I think we should use `struct.__opencl_sampler` as the name of sampler type in LLVM IR.

================
Comment at: lib/Sema/SemaInit.cpp:6949
@@ +6948,3 @@
+          // declaration.
+          if (!Var->getInit() || !isa<ImplicitCastExpr>(Var->getInit()))
+            break;
----------------
Anastasia wrote:
> Do we accept declaring global sampler_t variable without an initializer?
no. Since in file scope sampler variable is only allowed in constant addr space, it has to be initialized. There is a test for it.

================
Comment at: lib/Sema/SemaInit.cpp:6956
@@ +6955,3 @@
+        }
+      }
+
----------------
Anastasia wrote:
> Could you use else here instead of adding TakeLiteral variable to avoid going to the next compound statement?
Good catch. Will do.

================
Comment at: lib/Sema/SemaInit.cpp:6965
@@ +6964,3 @@
+        // the initializer.
+        if (!Init->isConstantInitializer(S.Context, false))
+          break;
----------------
Anastasia wrote:
> Should this generate an error?
This has already been diagnosed when parsing the intializer of the variable declaration.

================
Comment at: test/SemaOpenCL/sampler_t.cl:51
@@ -14,1 +50,3 @@
   foo(glb_smp);
+  foo(glb_smp2);
+  foo(glb_smp3);
----------------
Anastasia wrote:
> Are these extra calls below adding any extra testing?
Add the function calls to test

1. No additional error msgs since the error msgs should be emitted for the variable declarations.
2. no crashes when parsing them

================
Comment at: test/SemaOpenCL/sampler_t.cl:83
@@ -28,2 +82,3 @@
   *smp2; //expected-error{{invalid argument type 'sampler_t' to unary expression}}
+  foo(smp1+1); //expected-error{{invalid operands to binary expression ('sampler_t' and 'int')}}
 }
----------------
Anastasia wrote:
> Does this line add extra testing?
Test no crashes when an invalid sampler expression is passed as a sampler argument.


https://reviews.llvm.org/D21567