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