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. > > > > 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. > > 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. Thanks for the review! > -Eli > -- Thanks, Justin Holewinski
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
