<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, May 19, 2014 at 10:05 PM, 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="">----- Original Message -----<br>
> From: "Alp Toker" <<a href="mailto:alp@nuanti.com">alp@nuanti.com</a>><br>
> To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>>, "Richard Smith" <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
> Cc: <a href="mailto:reviews%2BD3806%2Bpublic%2B8377178d9ac8d89d@reviews.llvm.org">reviews+D3806+public+8377178d9ac8d89d@reviews.llvm.org</a>, "John McCall" <<a href="mailto:rjmccall@gmail.com">rjmccall@gmail.com</a>>, "llvm cfe"<br>
> <<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a>><br>
</div><div><div class="h5">> Sent: Monday, May 19, 2014 7:13:26 PM<br>
> Subject: Re: [PATCH] These builtin functions set errno. Mark them accordingly.<br>
><br>
><br>
> On 20/05/2014 01:13, Hal Finkel wrote:<br>
> > ----- Original Message -----<br>
> >> From: "Richard Smith" <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
> >> To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>><br>
> >> Cc: "Chandler Carruth" <<a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>>,<br>
> >> <a href="mailto:reviews%2BD3806%2Bpublic%2B8377178d9ac8d89d@reviews.llvm.org">reviews+D3806+public+8377178d9ac8d89d@reviews.llvm.org</a>, "John<br>
> >> McCall"<br>
> >> <<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 3:57:34 PM<br>
> >> Subject: Re: [PATCH] These builtin functions set errno. Mark them<br>
> >> accordingly.<br>
> >><br>
> >><br>
> >><br>
> >><br>
> >> On Mon, May 19, 2014 at 11:28 AM, Hal Finkel < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> ><br>
> >> wrote:<br>
> >><br>
> >><br>
> >><br>
> >><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" <<br>
> >>> <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<br>
> >>> accordingly.<br>
> >>><br>
> >>><br>
> >>><br>
> >>><br>
> >>><br>
> >>><br>
> >><br>
> >>> 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<br>
> >>> in<br>
> >>> Hal's words):<br>
> >>><br>
> >>> Many of these builtins turn into libc calls, and those do set<br>
> >>> 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();<br>
> >>> it<br>
> >>> may be reordered to write(); __builtin_sqrt(); perror(); and<br>
> >>> 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<br>
> >>> to<br>
> >>> force the direct lowering behavior even when we can't do it for<br>
> >>> sqrt<br>
> >>> because sqrt might set errno. But, as Hal said, this behavior<br>
> >>> *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<br>
> >>> the<br>
> >>> right way of handling it is. Hal thinks we really need some kind<br>
> >>> 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<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<br>
> >>> 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>
> >> Actually, I really like this idea. It will take some work,<br>
> >> however,<br>
> >> because errno is not a particular symbol (it is almost always a<br>
> >> macro that expands to something else), and it changes depending on<br>
> >> what OS/libc is being used. Clang has, at least in theory, all of<br>
> >> the information necessary to generate a save/restore function for<br>
> >> 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; }<br>
> >> __attribute__((always_inline))<br>
> >> static void __write_errno(int e) { errno = e; }<br>
> >> __attribute__((always_inline))");<br>
> >><br>
> >><br>
> >><br>
> >> I'm strongly opposed to this particular approach.<br>
> >><br>
> > The thing that I don't like about it is that it would affect the<br>
> > AST. We could, however, compile a small artificial source file<br>
> > containing just those definitions (using the same preprocessor<br>
> > defines, etc.), and just "link" the IR with the IR module for<br>
> > which we're actually generating code before we optimize (and we<br>
> > could do this only if some __builtin_<foo> was used). Do you feel<br>
> > differently about that?<br>
> ><br>
> >> Is it possible to teach LLVM's target library info about errno, or<br>
> >> does it vary too much (based on preprocessor defines / ...)?<br>
> > The problem, I think, really shows up on Linux systems where there<br>
> > is a choice of libc implementations (especially on the embedded<br>
> > side of things). Here's a quick overview of errno on different<br>
> > systems:<br>
> ><br>
> > - Traditional POSIX (not thread safe):<br>
> ><br>
> > extern int errno;<br>
> ><br>
> > - DragonFly libc:<br>
> ><br>
> > extern __thread int errno;<br>
> ><br>
> > - Android libc:<br>
> ><br>
> > extern volatile int* __errno(void);<br>
> > #define errno (*__errno())<br>
> ><br>
> > - Darwin libc:<br>
> ><br>
> > int * __error(void);<br>
> > #define errno (*__error())<br>
> ><br>
> > - musl libc:<br>
> ><br>
> > extern int *__errno_location(void);<br>
> > #define errno (*__errno_location())<br>
> ><br>
> > - glibc:<br>
> ><br>
> > extern int *__errno_location (void) __THROW __attribute__<br>
> > ((__const__));<br>
> > # if !defined _LIBC || defined _LIBC_REENTRANT<br>
> > define errno (*__errno_location ())<br>
> > # endif<br>
> ><br>
> > #ifndef errno<br>
> > extern int errno;<br>
> > #endif<br>
> ><br>
> > - newlib:<br>
> ><br>
> > #define errno (*__errno())<br>
> > extern int *__errno _PARAMS ((void));<br>
> ><br>
> > - diet libc<br>
> ><br>
> > #ifndef _REENTRANT<br>
> > extern int errno;<br>
> > #else<br>
> > #define errno (*__errno_location())<br>
> > #endif<br>
> ><br>
> > extern int *__errno_location(void);<br>
> ><br>
> > - uClibc<br>
> ><br>
> > #ifdef _LIBC<br>
> > #ifdef IS_IN_rtld<br>
> > # undef errno<br>
> > # define errno _dl_errno<br>
> > extern int _dl_errno; /* attribute_hidden */<br>
> > #elif defined __UCLIBC_HAS_TLS__<br>
> > # if !defined NOT_IN_libc || defined IS_IN_libpthread<br>
> > # undef errno<br>
> > # ifndef NOT_IN_libc<br>
> > # define errno __libc_errno<br>
> > # else<br>
> > # define errno errno /* For #ifndef errno tests.<br>
> > */<br>
> > # endif<br>
> > extern __thread int errno attribute_tls_model_ie;<br>
> > # endif<br>
> > #endif<br>
> ><br>
> > I'm somewhat afraid of introducing a dependence on the libc<br>
> > implementation at the LLVM target layer, and I'm somewhat afraid<br>
> > that it will break things specifically when Clang is being used to<br>
> > compile libc itself (or other system software). Having Clang<br>
> > provide the information seems like the "technically correct" way<br>
> > of doing it. Thoughts?<br>
><br>
> Perhaps we can adapt your __read_errno() / __write_errno() technique<br>
> to<br>
> make it less magic and by placing it it in a header like<br>
> lib/Headers/fastmath.h<br>
><br>
> User code including fastmath.h would become eligible for the goodies,<br>
> along with the errno.h or whatever include it took to achieve that.<br>
<br>
</div></div>The problem is that we're talking about how to generate code for __builtin_sqrt and friends. Having the behavior of __builtin_sqrt change when some header is included seems strange (although certainly possible), and certainly going to be different from gcc's behavior. Fundamentally, however, it takes something that is our problem (figuring out how to interact with errno), which we *can* solve within the compiler in a straightforward low-overhead way, and makes it the user's problem. Fundamentally, we should not do that.<br>
</blockquote><div><br></div><div>If we need to fall back to a libcall for a __builtin_blah function, we could perhaps fall back to something provided by compiler-rt (that never sets errno), rather than a libm function. The compiler-rt implementation could be as simple as saving errno across a call to libm.</div>
<div><br></div><div>Thoughts? What does GCC do? (If there's no libgcc equivalent, this is probably dead in the water since it won't work for people who don't use compiler-rt.)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks again,<br>
Hal<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
> Alp.<br>
><br>
><br>
> ><br>
> > Thanks again,<br>
> > Hal<br>
> ><br>
> >><br>
> >> 2. When we lower these __builtin_<foo> functions, add code around<br>
> >> 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<br>
> >> removes the save/restore code when it is dead (because the builtin<br>
> >> was lowered to an instruction) for common platforms.<br>
> >><br>
> >> -Hal<br>
> >><br>
> >>><br>
> >>> Too crazy?<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>
> >> --<br>
> >> Hal Finkel<br>
> >> Assistant Computational Scientist<br>
> >> Leadership Computing Facility<br>
> >> Argonne National Laboratory<br>
> >><br>
> >><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>
><br>
> --<br>
> <a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
> the browser experts<br>
><br>
><br>
<br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</div></div></blockquote></div><br></div></div>