Anastasia 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.
----------------
Could we update the comment here or otherwise I think it's best to be removed!
================
Comment at: lib/CodeGen/CGOpenCLRuntime.cpp:108
@@ +107,3 @@
+ PipeTy = llvm::PointerType::get(llvm::StructType::create(
+ CGM.getLLVMContext(), "opencl.pipe_t"), PipeAddrSpc);
+ }
----------------
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.
================
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}}
+}
----------------
I can see more than one diagnostic being added in this patch, but the test
covers only one.
http://reviews.llvm.org/D15603
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
