<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, May 19, 2014 at 11:28 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=""><br>
<br>
----- Original Message -----<br>
> From: "Chandler Carruth" <<a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>><br>
> To: <a href="mailto:reviews%2BD3806%2Bpublic%2B8377178d9ac8d89d@reviews.llvm.org">reviews+D3806+public+8377178d9ac8d89d@reviews.llvm.org</a><br>
> Cc: "John McCall" <<a href="mailto:rjmccall@gmail.com">rjmccall@gmail.com</a>>, "llvm cfe" <<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a>><br>
> Sent: Monday, May 19, 2014 12:35:47 PM<br>
> Subject: Re: [PATCH] These builtin functions set errno. Mark them accordingly.<br>
><br>
><br>
><br>
><br>
><br>
><br>
</div><div><div class="h5">> On Mon, May 19, 2014 at 10:26 AM, Chad Rosier <<br>
> <a href="mailto:mcrosier@codeaurora.org">mcrosier@codeaurora.org</a> > wrote:<br>
><br>
><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 errno.<br>
> 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(); it<br>
> may be reordered to write(); __builtin_sqrt(); perror(); and perror<br>
> may provide junk. However, if we don't mark the builtins as<br>
> 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 to<br>
> force the direct lowering behavior even when we can't do it for sqrt<br>
> because sqrt might set errno. But, as Hal said, this behavior *is*<br>
> broken, technically. And, in fact, it is not hard to create test<br>
> cases where you can see buggy behavior. Hak's just not sure what the<br>
> right way of handling it is. Hal thinks we really need some kind of<br>
> 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 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>
> Crazy idea:<br>
><br>
><br>
> When we lower one of these to a call to a libc function, save and<br>
> restore errno around it.<br>
<br>
</div></div>Actually, I really like this idea. It will take some work, however, because errno is not a particular symbol (it is almost always a macro that expands to something else), and it changes depending on what OS/libc is being used. Clang has, at least in theory, all of the information necessary to generate a save/restore function for errno (although it may need to force the inclusion of errno.h).<br>
<br>
For example, we could do something like this:<br>
<br>
1. In clang::InitializePreprocessor, add something like:<br>
Builder.append("#include <errno.h><br>
static int __read_errno() { return errno; } __attribute__((always_inline))<br>
static void __write_errno(int e) { errno = e; } __attribute__((always_inline))");<br></blockquote><div><br></div><div>I'm strongly opposed to this particular approach.</div><div><br></div><div>
Is it possible to teach LLVM's target library info about errno, or does it vary too much (based on preprocessor defines / ...)?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
2. When we lower these __builtin_<foo> functions, add code around them using __read_errno and __write_errno so that errno is not<br>
clobbered even if the builtin is lowered to a libm call.<br>
<br>
3. Make sure that DAGCombine and/or DeadMachineInstructionElim removes the save/restore code when it is dead (because the builtin was lowered to an instruction) for common platforms.<br>
<br>
-Hal<br>
<br>
><br>
><br>
> Too crazy?<br>
<div class="im HOEnZb">> _______________________________________________<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><div class="im HOEnZb">--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</div><div class="HOEnZb"><div class="h5">_______________________________________________<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></div>