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