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@-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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
