[PATCH] D14441: [OpenCL] Pipe types support.

[Alexey Bader via cfe-commits]

bader added inline comments.

================
Comment at: include/clang/Sema/DeclSpec.h:1426
@@ +1425,3 @@
+    struct PipeTypeInfo : TypeInfoCommon {
+    /// The access writes.
+    unsigned AccessWrites : 3;
----------------
pekka.jaaskelainen wrote:
> I think this needs a better comment than duplicating the variable name. What is the purpose of the variable?
It looks like it was added to handle pipe access qualifiers, but as far as I can see it doesn't do anything useful right now. I'll remove it.

================
Comment at: include/clang/Serialization/ASTBitCodes.h:911
@@ +910,3 @@
+      TYPE_ADJUSTED              = 42,
+      /// \brief An PipeType record.
+      TYPE_PIPE                  = 43
----------------
pekka.jaaskelainen wrote:
> A
No sure I understand that comment. Pekka, could you clarify, please?

================
Comment at: lib/AST/ASTContext.cpp:3141
@@ +3140,3 @@
+  }
+  PipeType *New = new (*this, TypeAlignment) PipeType(T, Canonical);
+  Types.push_back(New);
----------------
pekka.jaaskelainen wrote:
> Should we assign T to 'Canonical' if it already was Canonical or is this intentional somehow?
My understanding is that it's not required. T is used in most cases and 'Canonical' is queried only if T is not canonical. I looked at the similar methods of ASTContext - ASTContext::get*Type - they all pass default constructed 'Canonical' if original type is canonical.

================
Comment at: test/CodeGenOpenCL/pipe_types.cl:1
@@ +1,2 @@
+// RUN: %clang_cc1 -emit-llvm -O0 -cl-std=CL2.0 -o - %s | FileCheck %s
+
----------------
pekka.jaaskelainen wrote:
> Can you add a couple of more checks? For example:
> - the behavior when using CL1.2
> - when the inner type of the pipe has a 'const' or similar qualifier
> - It can only by a function arg. What if it's a local variable?
> 
I've added negative test that pipe types are not complied with CL1.2, but I'd like to note that this not the final patch. I'm also going to commit support for pipe built-ins and semantic checks as a separate patches.

================
Comment at: test/CodeGenOpenCL/pipe_types.cl:5
@@ +4,3 @@
+
+void test1(read_only pipe int p) {
+// CHECK: define void @test1(%opencl.pipe_t* %p)
----------------
pxli168 wrote:
> Great work!!
> But I have tried your patch and find it does not support opencl gentype like int4 with the usual used typedef in other test cases.
> Maybe there is something wrong?
I've added a test case for vector element pipes. It works for me. Could you send me you reproducer, please?


http://reviews.llvm.org/D14441