<br><br><div class="gmail_quote">On Wed, Apr 20, 2011 at 1:52 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;">
<div><div></div><div class="h5">On Wed, Apr 20, 2011 at 10:50 AM, Eli Friedman <<a href="mailto:eli.friedman@gmail.com">eli.friedman@gmail.com</a>> 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 the<br>
>> currently implemented PTX intrinsics as builtin functions.  I would like for<br>
>> someone familiar with the Clang targets/triples and builtins code to review<br>
>> this patch.  I have been involved in the PTX back-end for several 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></div></div></blockquote><div><br></div><div>Okay, will do.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div><div class="h5">

><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></div></div></blockquote><div><br></div><div>Would there be any issue with just removing the v64 and v128 specifiers for now?  The back-end does not currently support them.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div><div class="h5">
><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></div></div></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div><div class="h5">
<br>
</div></div>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></blockquote><div><br></div><div>Where exactly is this defined?  I'm not too familiar with Clang's TargetInfo class yet.</div><div><br></div><div>Thanks for the review! </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"><br>
-Eli<br>
</font></blockquote></div><br><br clear="all"><br>-- <br><br><div>Thanks,</div><div><br></div><div>Justin Holewinski</div><br>