[PATCH] D15603: [OpenCL] Pipe type support

[Anastasia Stulova via cfe-commits]

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?


http://reviews.llvm.org/D15603