Looks fine to me. -Tanya
On Dec 11, 2012, at 6:49 AM, "Benyei, Guy" <[email protected]> wrote: > Hi Eli, > Thanks for the review. > > Attached a fixed patch, please review. > >> + virtual bool hasFeature(StringRef Feature) const { >> + return Feature == "spir"; >> + } >> >> What's the point of this? > > This one is quite the same as in every virtual architecture, including > PNaCl/le32. I guess it is a convinient way to find out if the current target > implements this kind of abstraction. > > thanks > Guy Benyei > > > -----Original Message----- > From: Eli Friedman [mailto:[email protected]] > Sent: Tuesday, December 11, 2012 01:18 > To: Benyei, Guy > Cc: [email protected] > Subject: Re: [cfe-commits] [Patch] SPIR32/SPIR64 targets in Clang > > On Sun, Dec 9, 2012 at 1:30 AM, Benyei, Guy <[email protected]> wrote: >> >> As I've got no response up until now, I'm re-sending this patch - >> removed some target defines that probably should be defined in the >> pre-processor initialization. >> >> >> >> I think this is a fairly simple patch. > > Sorry about the delay; I was out last week. > > + assert(getTriple().getOS() == llvm::Triple::UnknownOS && > + "SPIR target must use unknown OS"); > + assert(getTriple().getEnvironment() == > llvm::Triple::UnknownEnvironment && > + "SPIR target must use unknown environment type"); > > Don't assert something we don't enforce. If you really want to enforce it, > check before the "new SPIR32TargetInfo(T)". > > + virtual bool hasFeature(StringRef Feature) const { > + return Feature == "spir"; > + } > > What's the point of this? > > + virtual const char *getVAListDeclaration() const { > + return ""; > + } > > How did this sneak in? > > Index: test/CodeGenOpenCL/spir32_target.cl > =================================================================== > --- test/CodeGenOpenCL/spir32_target.cl (revision 0) > +++ test/CodeGenOpenCL/spir32_target.cl (revision 0) > @@ -0,0 +1,24 @@ > +// RUN: %clang_cc1 %s -triple "spir-unknown-unknown" -emit-llvm -o - > | FileCheck %s > + > +// CHECK: target triple = "spir-unknown-unknown" > + > +typedef struct { > + char c; > + void *v; > + void *v2; > +} my_st; > + > +kernel void foo(global long *arg) { > + arg[0] = sizeof(my_st); > +//CHECK: store i64 12, i64 addrspace(1)* > > We generally prefer to write this sort of test using _Static_Assert or > equivalent. > > -Eli > --------------------------------------------------------------------- > Intel Israel (74) Limited > > This e-mail and any attachments may contain confidential material for > the sole use of the intended recipient(s). Any review or distribution > by others is strictly prohibited. If you are not the intended > recipient, please contact the sender and delete all copies. > <SPIRtarget3.patch>_______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
