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