----- Original Message ----- > From: "Raul Silvera" <rsilv...@google.com> > To: "Hal Finkel" <hfin...@anl.gov> > Cc: reviews+d3806+public+8377178d9ac8d...@reviews.llvm.org, > rjmcc...@gmail.com, "llvm cfe" <cfe-commits@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 | rsilv...@google.com | 408-789-2846 > > > > On Mon, May 19, 2014 at 10:05 AM, Hal Finkel < hfin...@anl.gov > > wrote: > > > > > ----- Original Message ----- > > From: "Chris Lattner" < clatt...@apple.com > > > To: reviews+d3806+public+8377178d9ac8d...@reviews.llvm.org > > Cc: rjmcc...@gmail.com , cfe-commits@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 < mcros...@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@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@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@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
