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

[Hal Finkel]

----- Original Message -----
> From: "Raul Silvera" <rsilvera at google.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: reviews+D3806+public+8377178d9ac8d89d at reviews.llvm.org, rjmccall at gmail.com, "llvm cfe" <cfe-commits at cs.uiuc.edu>
> Sent: Monday, May 19, 2014 12:45:34 PM
> Subject: Re: [PATCH] These builtin functions set errno. Mark them accordingly.
> 
> 
> 
> 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.
> 
> 

I agree with this; however, you'd still end up pessimizing uses around function calls because of potential clobbers. I would, however, very much like to track uses of errno so that we can declare many such dependencies dead (especially after LTO).

 -Hal

> 
> 
> 
> 
> 
> 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
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory