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

[Eli Friedman]

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.

-Eli