<div class="gmail_quote">On Wed, Apr 20, 2011 at 2:17 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 10:58 AM, Justin Holewinski<br>
<div><div></div><div class="h5"><<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 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>
>> > +      PointerWidth = PointerAlign = 32;<br>
>> > +      DescriptionString<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 for<br>
> now?  The back-end does not currently support them.<br>
<br>
</div></div>That's fine.<br>
<div class="im"><br>
>><br>
>> ><br>
>> > BUILTIN(__builtin_ptx_read_tid_x, "i", "nc")<br>
>> ><br>
>> > The "c" means that the value returned by these builtins never changes;<br>
>> > that doesn't seem right.<br>
>> ><br>
><br>
> This is an interesting case.  I assumed the "const" meant that successive<br>
> calls to the builtin will return the same value (which is true).  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 PTX,<br>
> it is perfectly valid for constant propagation to eliminate any successive<br>
> calls.<br>
<br>
</div>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></blockquote><div><br></div><div>Oops, that is a copy-paste error.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im"><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 TargetInfo<br>
> class yet.<br>
<br>
</div>SizeType, PtrDiffType, IntPtrType.<br></blockquote><div><br></div><div>Fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<font color="#888888"><br>
-Eli<br>
</font></blockquote></div><br>Attached is an updated patch.<br clear="all"><br>-- <br><br><div>Thanks,</div><div><br></div><div>Justin Holewinski</div><br>