[PATCH] D15603: [OpenCL] Pipe type support

Anastasia Stulova via cfe-commits cfe-commits at lists.llvm.org
Thu Dec 24 08:33:32 PST 2015

Anastasia added inline comments.

Comment at: lib/AST/ASTContext.cpp:3121
@@ +3120,3 @@
+QualType ASTContext::getPipeType(QualType T) const {
+  // Unique pointers, to guarantee there is only one pointer of a particular
+  // structure.
I don't think this comment is much related here - a copy-paste issue perhaps? But I see this is replicated elsewhere.

As mentioned in comment of http://reviews.llvm.org/D14441, I think we might want to look at refactoring the code that handles wrapper types in Clang. However, this shouldn't prevent this patch from being committed.

Comment at: lib/CodeGen/CGOpenCLRuntime.cpp:108
@@ +107,3 @@
+    PipeTy = llvm::PointerType::get(llvm::StructType::create(
+      CGM.getLLVMContext(), "opencl.pipe_t"), PipeAddrSpc);
+  }
Should there be any indication of pipe element type or at least its size?  I.e should we differentiate between 'pipe char' or 'pipe int' in IR?

Perhaps, it doesn't have to be a part of this commit, but it would still be useful to understand.

Comment at: lib/CodeGen/CodeGenFunction.cpp:565
@@ +564,3 @@
+    // FIXME: now image and pipe share the same access qualifier maybe we can
+    // refine it to OCL acess qualifier and also handle write_read
+    if (ty->isImageType()|| ty->isPipeType()) {
acess -> access
OCL -> OpenCL

Comment at: lib/Sema/TreeTransform.h:5352
@@ -5329,1 +5351,3 @@
+namespace {
   /// \brief Simple iterator that traverses the template arguments in a
Not clear why the namespace is being added here? i. e. seems to be old code.

Comment at: test/CodeGenOpenCL/pipe_types.cl:15
@@ +14,3 @@
+// CHECK: define void @test2(%opencl.pipe_t* %p)
+  reserve_id_t rid;
+// CHECK: %rid = alloca %opencl.reserve_id_t
Do we need to check reserve_id_t in every test? I feel like having it once would be enough.

Comment at: test/SemaOpenCL/pipes-1.2-negative.cl:1
@@ +1,2 @@
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL1.2
Could we add tests for checking CL2.0 pipes diagnostics added in this patch?


More information about the cfe-commits mailing list