[cfe-commits] [PATCH] Add ptx32 and ptx64 target triples, as well as PTX builtins

[Justin Holewinski]

On Wed, Apr 20, 2011 at 3:18 PM, Eli Friedman <eli.friedman at gmail.com>wrote:

> On Wed, Apr 20, 2011 at 12:15 PM, Justin Holewinski
> <justin.holewinski at gmail.com> wrote:
> > On Wed, Apr 20, 2011 at 2:17 PM, Eli Friedman <eli.friedman at gmail.com>
> > wrote:
> >>
> >> On Wed, Apr 20, 2011 at 10:58 AM, Justin Holewinski
> >> <justin.holewinski at gmail.com> wrote:
> >> >
> >> >
> >> > On Wed, Apr 20, 2011 at 1:52 PM, Eli Friedman <eli.friedman at gmail.com
> >
> >> > wrote:
> >> >>
> >> >> On Wed, Apr 20, 2011 at 10:50 AM, Eli Friedman <
> eli.friedman at gmail.com>
> >> >> wrote:
> >> >> > On Wed, Apr 20, 2011 at 10:14 AM, Justin Holewinski
> >> >> > <justin.holewinski at gmail.com> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110420/d96b6196/attachment.html>