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

[Anastasia Stulova via cfe-commits]

Anastasia added a comment.

Btw, I am missing tests for generated __translate_sampler_initializer which I think we had at some point.


================
Comment at: lib/CodeGen/CGOpenCLRuntime.cpp:84
@@ +83,3 @@
+    SamplerTy = llvm::PointerType::get(llvm::StructType::create(
+      CGM.getLLVMContext(), "__opencl_sampler_t"),
+      CGM.getContext().getTargetAddressSpace(
----------------
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.

================
Comment at: lib/Sema/SemaInit.cpp:6907
@@ -6905,2 +6906,3 @@
     case SK_OCLSamplerInit: {
-      assert(Step->Type->isSamplerT() && 
+      // Sampler initialzation have 6 cases:
+      //   1. function argument passing
----------------
There are 5 cases it seems :)

================
Comment at: lib/Sema/SemaInit.cpp:6947
@@ +6946,3 @@
+          // Do not diagnose if the file-scope variable does not have initializer
+          // since this has already beend diagnosed when parsing the variable
+          // declaration.
----------------
beend -> been

================
Comment at: lib/Sema/SemaInit.cpp:6949
@@ +6948,3 @@
+          // declaration.
+          if (!Var->getInit() || !isa<ImplicitCastExpr>(Var->getInit()))
+            break;
----------------
Do we accept declaring global sampler_t variable without an initializer?

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

================
Comment at: lib/Sema/SemaInit.cpp:6965
@@ +6964,3 @@
+        // the initializer.
+        if (!Init->isConstantInitializer(S.Context, false))
+          break;
----------------
Should this generate an error?

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

================
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')}}
 }
----------------
Does this line add extra testing?


https://reviews.llvm.org/D21567