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

[Anastasia Stulova via cfe-commits]

Anastasia added inline comments.

================
Comment at: lib/CodeGen/CGOpenCLRuntime.cpp:83
@@ +82,3 @@
+    SamplerTy = llvm::PointerType::get(llvm::StructType::create(
+      CGM.getLLVMContext(), "__sampler"),
+      CGM.getContext().getTargetAddressSpace(
----------------
I am wondering if we could at least name it __opencl_sampler_t. I kind of don't like that we have inconsistency with the rest here.

Btw, the types in Debug are all named opencl_<type>. Perhaps we could correct them consistently across?

================
Comment at: lib/CodeGen/CodeGenModule.cpp:4309
@@ +4308,3 @@
+  // in SPIR spec v1.2 and also opencl-c.h
+  unsigned AddressingMode  = (0x0E & SamplerValue) >> 1;
+  unsigned FilterMode      = (0x30 & SamplerValue) >> 4;
----------------
Yes sounds good!

================
Comment at: lib/Sema/SemaInit.cpp:6907
@@ -6906,3 +6907,1 @@
     case SK_OCLSamplerInit: {
-      assert(Step->Type->isSamplerT() && 
-             "Sampler initialization on non-sampler type.");
----------------
But I am guessing that holds for most of the assertions. :)

================
Comment at: lib/Sema/SemaInit.cpp:6915
@@ -6915,3 +6914,3 @@
             << SourceType;
-      } else if (Entity.getKind() != InitializedEntity::EK_Variable) {
-        llvm_unreachable("Invalid EntityKind!");
+        } else if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Init)) {
+          Init = const_cast<Expr*>(cast<VarDecl>(DRE->getDecl())->getInit());
----------------
Should you also check for type of Expr to be sampler? 

Btw is this covered in testing?

================
Comment at: test/CodeGenOpenCL/sampler.cl:1
@@ +1,2 @@
+// RUN: %clang_cc1 %s -emit-llvm -triple spir-unknown-unknown -o - -O0 -Wspir-compat -verify -DCHECK_SAMPLER_VALUE | FileCheck %s
+// RUN: %clang_cc1 %s -emit-llvm -triple spir-unknown-unknown -o - -O0 -verify | FileCheck %s
----------------
I think we can give this warning for spir target or -Wspir-compat.

================
Comment at: test/CodeGenOpenCL/sampler.cl:17
@@ +16,3 @@
+#ifdef CHECK_SAMPLER_VALUE
+// expected-warning at -2{{sampler initializer has invalid Filter Mode bits}}
+#endif
----------------
This should go into SemaOpenCL tests!

================
Comment at: test/CodeGenOpenCL/sampler.cl:27
@@ +26,3 @@
+
+void fnc4smp(sampler_t s) {}
+// CHECK: define spir_func void @fnc4smp(%__sampler addrspace(2)* %
----------------
Could we add CHECK-NOT respectively?

================
Comment at: test/CodeGenOpenCL/sampler.cl:37
@@ +36,3 @@
+
+  fnc4smp(smp);
+  // CHECK: [[SAMP:%[0-9]+]] = call %__sampler addrspace(2)* @__translate_sampler_initializer(i32 19)
----------------
Can we add another call with smp and check that __translate_sampler_initializeris is not called again?


http://reviews.llvm.org/D21567