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