[PATCH] These builtin functions set errno. Mark them accordingly.

[Raul Silvera]

I think that the issue is the distinction between setting and clobbering
errno.

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.

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.


Raúl E Silvera | SWE | rsilvera at google.com | *408-789-2846*



On Mon, May 19, 2014 at 10:05 AM, Hal Finkel <hfinkel at anl.gov> wrote:

> ----- Original Message -----
> > From: "Chris Lattner" <clattner at apple.com>
> > To: reviews+D3806+public+8377178d9ac8d89d at reviews.llvm.org
> > Cc: rjmccall at gmail.com, cfe-commits at cs.uiuc.edu
> > Sent: Monday, May 19, 2014 11:51:30 AM
> > Subject: Re: [PATCH] These builtin functions set errno. Mark them
> accordingly.
> >
> >
> > On May 19, 2014, at 9:26 AM, Chad Rosier <mcrosier at codeaurora.org>
> > wrote:
> >
> > > Hal and I discussed this on IRC and here's the synopsis (mostly in
> > > Hal's words):
> > >
> > > Many of these builtins turn into libc calls, and those do set
> > > errno. Marking them as readnone is a problem, because we can
> > > reorder/eliminate writes to errno.  For example, if we assume
> > > readnone for something like __builtin_sqrt(); write(); perror();
> > > it may be reordered to write(); __builtin_sqrt(); perror(); and
> > > perror may provide junk.  However, if we don't mark the builtins
> > > as readnone, then they won't get lowered into ISD nodes (for those
> > > nodes that are relevant), and that means that they won't get
> > > instruction selected into native instructions on targets where
> > > that's possible. So the __builtin_sqrt is kind of an escape hatch
> > > to force the direct lowering behavior even when we can't do it for
> > > sqrt because sqrt might set errno.  But, as Hal said, this
> > > behavior *is* broken, technically. And, in fact, it is not hard to
> > > create test cases where you can see buggy behavior.  Hak's just
> > > not sure what the right way of handling it is. Hal thinks we
> > > really need some kind of target inp!
> > > ut, so that we can know ahead of time whether the builtin will
> > > *really* set errno or not after isel.  Hal thinks there are two
> > > solutions possible.
> > >
> > > 1. Like with inline asm registers/constraints; each target will
> > > provide a table describing what builtins will really not set
> > > errno.
> > > 2. Let Clang essentially query TTI for those builtins that are
> > > relevant. This second method will require a bit of infrastructure
> > > work, but is probably not too difficult.
> > >
> > > I'm not going to push this patch further because it's not the right
> > > solution.  In theory, I believe it is correct, but it breaks the
> > > __buitlin escape hatch, which would likely result in serious
> > > performance degradations.
> >
> > I don’t think this is a good solution.  The better option here is to
> > tell people to use -fno-math-errno if they don’t care about libm
> > functions setting errno.
>
> 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*.
>
>  -Hal
>
> >
> > -Chris
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140519/f40b1bbf/attachment.html>