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

Yaxun Liu via cfe-commits cfe-commits at lists.llvm.org
Wed Jul 13 13:34:44 PDT 2016


yaxunl marked 11 inline comments as done.

================
Comment at: lib/CodeGen/CGOpenCLRuntime.cpp:83
@@ +82,3 @@
+    SamplerTy = llvm::PointerType::get(llvm::StructType::create(
+      CGM.getLLVMContext(), "__sampler"),
+      CGM.getContext().getTargetAddressSpace(
----------------
Anastasia wrote:
> 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?
I will rename it as __opencl_sampler_t.

I would suggest to rename all opencl types as __opencl_x instead of opencl.x (in another patch) to avoid unnecessary casts in libraries.

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

================
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());
----------------
Anastasia wrote:
> Should you also check for type of Expr to be sampler? 
> 
> Btw is this covered in testing?
The type of Init is SourceType. We already checked it in line 6912.

Here we want to handle the case where a sampler type variable is passed as function argument. Since DRE->getDecl() has the same type as DRE, so we don't need to check that.

Since we don't allow operators on sampler, the only sampler expressions we can get are

1. sampler type global variable
2. sampler type function-scope variable
3. sampler type function parameter

I covered the 3 cases in the tests. Also covered passing non-sampler type expression as sampler argument to a function. However I noticed I did not cover the cases that sampler type variable without initializer. I will add tests for that.

================
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
----------------
Anastasia wrote:
> I think we can give this warning for spir target or -Wspir-compat.
Sorry I did not turn on the warning for spir-compat by default for SPIR target. As I checked the part of code about warnings, I did not find a precedence to turn on warning for a specific target? So here I still need -Wspir-compat to see the warnings for SPIR target. Should I turn on the warning for spir-compat by default for SPIR target.

================
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
----------------
Anastasia wrote:
> This should go into SemaOpenCL tests!
This warning is emitted at codegen stage instead of parsing since diagnosing this requires evaluated value of the initializer expression, which is readily available at codegen, but need some extra effort to obtain at parsing.

One side effect is that we will only see the warning if the test does not contain other sema errors because otherwise we will not reach codegen stage. That's why I test it in a codegen test since the sema tests contain other errors.

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

================
Comment at: test/CodeGenOpenCL/sampler.cl:37
@@ +36,3 @@
+
+  fnc4smp(smp);
+  // CHECK: [[SAMP:%[0-9]+]] = call %__sampler addrspace(2)* @__translate_sampler_initializer(i32 19)
----------------
Anastasia wrote:
> Can we add another call with smp and check that __translate_sampler_initializeris is not called again?
Will do. Currently it generates call for each reference of smp, but I will fix that.


http://reviews.llvm.org/D21567





More information about the cfe-commits mailing list