On Wed, Apr 20, 2011 at 3:18 PM, Eli Friedman <[email protected]>wrote:
> On Wed, Apr 20, 2011 at 12:15 PM, Justin Holewinski > <[email protected]> wrote: > > On Wed, Apr 20, 2011 at 2:17 PM, Eli Friedman <[email protected]> > > wrote: > >> > >> On Wed, Apr 20, 2011 at 10:58 AM, Justin Holewinski > >> <[email protected]> wrote: > >> > > >> > > >> > On Wed, Apr 20, 2011 at 1:52 PM, Eli Friedman <[email protected] > > > >> > wrote: > >> >> > >> >> On Wed, Apr 20, 2011 at 10:50 AM, Eli Friedman < > [email protected]> > >> >> wrote: > >> >> > On Wed, Apr 20, 2011 at 10:14 AM, Justin Holewinski > >> >> > <[email protected]> wrote: > >> >> >> The attached patch adds target triples for the PTX back-end, and > >> >> >> adds > >> >> >> the > >> >> >> currently implemented PTX intrinsics as builtin functions. I > would > >> >> >> like for > >> >> >> someone familiar with the Clang targets/triples and builtins code > to > >> >> >> review > >> >> >> this patch. I have been involved in the PTX back-end for several > >> >> >> months > >> >> >> now, but this is my first patch to Clang. > >> >> > > >> >> > + PTXTargetInfo(const std::string& triple) : TargetInfo(triple) > { > >> >> > + TLSSupported = false; > >> >> > + IntWidth = IntAlign = 32; > >> >> > + LongWidth = LongLongWidth = LongAlign = LongLongAlign = 64; > >> >> > + } > >> >> > > >> >> > You can skip setting IntWidth/IntAlign/LongLongWidth/LongLongAlign; > >> >> > the defaults are correct. > >> > > >> > Okay, will do. > >> > > >> >> > >> >> > > >> >> > + class PTX32TargetInfo : public PTXTargetInfo { > >> >> > + public: > >> >> > + PTX32TargetInfo(const std::string& triple) : > PTXTargetInfo(triple) > >> >> > { > >> >> > + PointerWidth = PointerAlign = 32; > >> >> > + DescriptionString > >> >> > + = > >> >> > "e-p:32:32-i64:32:32-f64:32:32-v128:32:128-v64:32:64-n32:64"; > >> >> > + } > >> >> > + }; > >> >> > > >> >> > The target description string isn't using the same alignment for > long > >> >> > and for double as the clang TargetInfo. Also, the alignment for > v64 > >> >> > and v128 looks wrong. > >> > > >> > Would there be any issue with just removing the v64 and v128 > specifiers > >> > for > >> > now? The back-end does not currently support them. > >> > >> That's fine. > >> > >> >> > >> >> > > >> >> > BUILTIN(__builtin_ptx_read_tid_x, "i", "nc") > >> >> > > >> >> > The "c" means that the value returned by these builtins never > >> >> > changes; > >> >> > that doesn't seem right. > >> >> > > >> > > >> > This is an interesting case. I assumed the "const" meant that > >> > successive > >> > calls to the builtin will return the same value (which is true). > >> > However, > >> > different threads will see different values. What is the recommended > >> > behavior here? For a given piece of C code that will compile down to > >> > PTX, > >> > it is perfectly valid for constant propagation to eliminate any > >> > successive > >> > calls. > >> > >> It should be fine for __builtin_ptx_read_tid_x, but it seems a bit > >> weird for __builtin_ptx_read_clock, and very wrong for > >> __builtin_ptx_bar_sync. > > > > Oops, that is a copy-paste error. > > > >> > >> >> > >> >> Oh, one more thing: this description unconditionally defines size_t > >> >> and friends to a 64-bit type; that isn't wrong, exactly, but probably > >> >> not what you want. > >> > > >> > Where exactly is this defined? I'm not too familiar with Clang's > >> > TargetInfo > >> > class yet. > >> > >> SizeType, PtrDiffType, IntPtrType. > > > > Fixed. > > > >> > >> -Eli > > > > Attached is an updated patch. > > Looks good. > Thanks for the review! Commited in r129870. -Eli > -- Thanks, Justin Holewinski
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
