[PATCH] D15603: [OpenCL] Pipe type support

[Xiuli PAN via cfe-commits]

pxli168 added inline comments.

================
Comment at: lib/CodeGen/CGOpenCLRuntime.cpp:108
@@ +107,3 @@
+    PipeTy = llvm::PointerType::get(llvm::StructType::create(
+      CGM.getLLVMContext(), "opencl.pipe_t"), PipeAddrSpc);
+  }
----------------
Anastasia wrote:
> 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.
There is one for the element size in the pipe related built-ins. But the built-in part refined the some of the built-in functions.
For example, the original read_pipe with 2 parameters is in the form

```
int read_pipe(pipe gentype p, gentype *e);
```
To get the indication for the pipe element type with in functions, we change it into this form

```
int read_pipe(pipe gentype p, char *e,int sizeofgentype);
```
This actually change the form of the OpenCL built-in functions, but can save a lot of things for the backend.
Another way is to get the pipe element type form the -cl-kernel-arg-info, the pipe is as the type qual and the element type for the type.
So which one do you think is better? Add a size or using kernel-arg-info, I am now wondering if the built-in for pipe is really needed. Hope you can give me some advice.

================
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()) {
----------------
Anastasia wrote:
> acess -> access
> OCL -> OpenCL
I will fix them, thank you.

================
Comment at: lib/Sema/TreeTransform.h:5352
@@ -5329,1 +5351,3 @@
+
+namespace {
   /// \brief Simple iterator that traverses the template arguments in a
----------------
Anastasia wrote:
> Not clear why the namespace is being added here? i. e. seems to be old code.
It seems to have no use, I will try to remove it and see if it can compile.

================
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
----------------
Anastasia wrote:
> Do we need to check reserve_id_t in every test? I feel like having it once would be enough.
Good idea, I will remove some.

================
Comment at: test/SemaOpenCL/pipes-1.2-negative.cl:1
@@ +1,2 @@
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL1.2
+
----------------
Anastasia wrote:
> Could we add tests for checking CL2.0 pipes diagnostics added in this patch?
Ok, I will add those checking for pipe access qualifier and element type.


http://reviews.llvm.org/D15603