[PATCH] D15603: [OpenCL] Pipe type support

[Xiuli PAN via cfe-commits]

pxli168 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.
----------------
Anastasia wrote:
> Could we update the comment here or otherwise I think it's best to be removed!
OK, just removed the copy paste one

================
Comment at: lib/CodeGen/CGOpenCLRuntime.cpp:108
@@ +107,3 @@
+    PipeTy = llvm::PointerType::get(llvm::StructType::create(
+      CGM.getLLVMContext(), "opencl.pipe_t"), PipeAddrSpc);
+  }
----------------
Anastasia wrote:
> pxli168 wrote:
> > pekka.jaaskelainen wrote:
> > > pxli168 wrote:
> > > > pekka.jaaskelainen wrote:
> > > > > I'm not sure if touching the built-in fingerprints for this is a good idea. It might lead to problems and confusion. Cannot one pass pipes as arguments to user functions too? Are the fingerprints of those functions then modified accordingly? It might become messy.
> > > > > 
> > > > > Because the packet size is known at host side, the pipes can be implemented as structs which holds the packet size as one of the member variables. The problem with this approach is how to exploit wider reads/writes instead of a scalar read/write loop + unpack/pack in case of vector datatypes. 
> > > > > 
> > > > > If the size is known only at runtime, one cannot easily generate vector reads/writes even if the element is a vector datatype and it would be efficient to keep the packet in a vector register as long as possible. For helping this I'd add a metadata which can be utilized at compile time to make reading/writing from the pipe faster.  But in a way that is already an optimization, not a requirement, to make pipes work.
> > > > > 
> > > > > The reading itself is platform dependent as the pipe can be even a hardware FIFO accessed using special instructions.
> > > > This is what I'm worry about, I don't think we need to give much information about an opaque type in OpenCL.
> > > > 
> > > > Actually we can get the element type from the metadata, and I think we can leave the optimization to the backend and let platform to choose which way to use for read/write pipe.
> > > > 
> > > > And I think the built-in function support for the pipe in OpenCL-C is not necessary in the clang, what do you think? Though it can do some check in Sema check, they can also be done in some llvm pass in backend. If the built-in is really needed, I will send another patch based on this included built-in support for pipe.
> > > > 
> > > > Thank you.
> > > As far as I've understood, no there's no need to add built-in function awareness to the frontend for this case. Sema checking/diagnostics is needed to ensure pipes are used only as function arguments, not local variables, for example. Is this patch missing it?
> > I think there are a lot of Sema checking about OpenCL2.0 are missing, the pipe could not be a local variable is just one, there are also some check are missing for reserve_id_t in my tests these days. 
> > My plan is to use a patch to cover all these missing Sema check for OpenCL. That would be a hard work with the OpenCL Specification and would take a lot of time and may delay this patch, so could you accept this patch first and let me to finish the missing Sema check later to make sure cover more missing part.
> I am Ok with not adding everything in one commit. We can revisit/modify things later on.
Do you think we need the make the read/write_pipe as a builtin in clang? Since these function need the pipe element type to be the same with the pointer pointee type. I find it hard to handle this in a backend.

================
Comment at: test/SemaOpenCL/invalid-pipes-cl2.0.cl:3
@@ +2,3 @@
+
+void test(read_only pipe int *p){// expected-error {{pipes packet types cannot be of reference type}}
+}
----------------
Anastasia wrote:
> I can see more than one diagnostic being added in this patch, but the test covers only one.
It seems the auto related diagnostic is not necessary, since auto can not be used in OpenCL.

And the  "err_missing_actual_pipe_type" seems is used in a wrong place, it could not handle something like 
```pipe p
```
This will case "warning: type specifier missing, defaults to 'int'"

But something like

```
int pipe p
```
will has  "error: missing actual type specifier for pipe"
It seems this should be call “cannot combine with previous 'int' declaration specifier”
And the "error: missing actual type specifier for pipe" should be placed somewhere else.


http://reviews.llvm.org/D15603