<div dir="ltr"><div class="gmail_default" style="font-family:verdana,sans-serif">I think that the issue is the distinction between setting and clobbering errno.</div><div class="gmail_default" style="font-family:verdana,sans-serif">
<br></div><div class="gmail_default" style="font-family:verdana,sans-serif">Marking builtin_sqrt as ReadNone is correct in the sense that code should not rely on errno being set by calls to the intrinsic. What is wrong is to expect errno to be preserved by the call, as we want to reserve the option of implementing it as a libm call.</div>
<div class="gmail_default" style="font-family:verdana,sans-serif"><br></div><div class="gmail_default" style="font-family:verdana,sans-serif">I believe modelling these semantics (which could possibly require a new attribute), would enable exploitation of the important optimization opportunities while avoiding the problematic cases.</div>
<div class="gmail_default" style="font-family:verdana,sans-serif"><br></div></div><div class="gmail_extra"><br clear="all"><div><div dir="ltr"><div><div style="line-height:1.5em;padding-top:10px;margin-top:10px;font-family:sans-serif;font-size:small">
<span style="color:rgb(85,85,85);border-width:2px 0px 0px;border-style:solid;border-color:rgb(213,15,37);padding-top:2px;margin-top:2px">Raúl E Silvera |</span><span style="color:rgb(85,85,85);border-width:2px 0px 0px;border-style:solid;border-color:rgb(51,105,232);padding-top:2px;margin-top:2px"> SWE |</span><span style="color:rgb(85,85,85);border-width:2px 0px 0px;border-style:solid;border-color:rgb(0,153,57);padding-top:2px;margin-top:2px"> <a href="mailto:rsilvera@google.com" target="_blank">rsilvera@google.com</a> |</span><span style="border-width:2px 0px 0px;border-style:solid;border-color:rgb(238,178,17);padding-top:2px;margin-top:2px"><font color="#555555"> </font><font color="#0033bb"><u>408-789-2846</u></font></span></div>
<br></div></div></div>
<br><br><div class="gmail_quote">On Mon, May 19, 2014 at 10:05 AM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">----- Original Message -----<br>
> From: "Chris Lattner" <<a href="mailto:clattner@apple.com">clattner@apple.com</a>><br>
> To: <a href="mailto:reviews%2BD3806%2Bpublic%2B8377178d9ac8d89d@reviews.llvm.org">reviews+D3806+public+8377178d9ac8d89d@reviews.llvm.org</a><br>
> Cc: <a href="mailto:rjmccall@gmail.com">rjmccall@gmail.com</a>, <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> Sent: Monday, May 19, 2014 11:51:30 AM<br>
> Subject: Re: [PATCH] These builtin functions set errno. Mark them accordingly.<br>
><br>
><br>
> On May 19, 2014, at 9:26 AM, Chad Rosier <<a href="mailto:mcrosier@codeaurora.org">mcrosier@codeaurora.org</a>><br>
> wrote:<br>
><br>
> > Hal and I discussed this on IRC and here's the synopsis (mostly in<br>
> > Hal's words):<br>
> ><br>
> > Many of these builtins turn into libc calls, and those do set<br>
> > errno. Marking them as readnone is a problem, because we can<br>
> > reorder/eliminate writes to errno. For example, if we assume<br>
> > readnone for something like __builtin_sqrt(); write(); perror();<br>
> > it may be reordered to write(); __builtin_sqrt(); perror(); and<br>
> > perror may provide junk. However, if we don't mark the builtins<br>
> > as readnone, then they won't get lowered into ISD nodes (for those<br>
> > nodes that are relevant), and that means that they won't get<br>
> > instruction selected into native instructions on targets where<br>
> > that's possible. So the __builtin_sqrt is kind of an escape hatch<br>
> > to force the direct lowering behavior even when we can't do it for<br>
> > sqrt because sqrt might set errno. But, as Hal said, this<br>
> > behavior *is* broken, technically. And, in fact, it is not hard to<br>
> > create test cases where you can see buggy behavior. Hak's just<br>
> > not sure what the right way of handling it is. Hal thinks we<br>
> > really need some kind of target inp!<br>
> > ut, so that we can know ahead of time whether the builtin will<br>
> > *really* set errno or not after isel. Hal thinks there are two<br>
> > solutions possible.<br>
> ><br>
> > 1. Like with inline asm registers/constraints; each target will<br>
> > provide a table describing what builtins will really not set<br>
> > errno.<br>
> > 2. Let Clang essentially query TTI for those builtins that are<br>
> > relevant. This second method will require a bit of infrastructure<br>
> > work, but is probably not too difficult.<br>
> ><br>
> > I'm not going to push this patch further because it's not the right<br>
> > solution. In theory, I believe it is correct, but it breaks the<br>
> > __buitlin escape hatch, which would likely result in serious<br>
> > performance degradations.<br>
><br>
> I don’t think this is a good solution. The better option here is to<br>
> tell people to use -fno-math-errno if they don’t care about libm<br>
> functions setting errno.<br>
<br>
</div></div>But this is the chicken and egg problem: is __builtin_sqrt a libm function? The problem that we currently have, IIUC, is that the answer is *sometimes*.<br>
<br>
-Hal<br>
<div class="im HOEnZb"><br>
><br>
> -Chris<br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
><br>
<br>
</div><span class="HOEnZb"><font color="#888888">--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div>