[PATCH] D24816: [Target] move reciprocal estimate settings from TargetOptions to TargetLowering

[Hal Finkel via llvm-commits]

----- Original Message -----

> From: "Eric Christopher" <echristo at gmail.com>
> To: reviews+D24816+public+f0b666a7aa70bda2 at reviews.llvm.org,
> spatel at rotateright.com, "e menezes" <e.menezes at samsung.com>,
> hfinkel at anl.gov
> Cc: mcrosier at codeaurora.org, "mehdi amini" <mehdi.amini at apple.com>,
> "nemanja i ibm" <nemanja.i.ibm at gmail.com>,
> llvm-commits at lists.llvm.org
> Sent: Monday, October 3, 2016 2:39:56 AM
> Subject: Re: [PATCH] D24816: [Target] move reciprocal estimate
> settings from TargetOptions to TargetLowering

> On Sun, Oct 2, 2016 at 7:27 PM Hal Finkel < hfinkel at anl.gov > wrote:

> > hfinkel added a comment.
> 

> > In https://reviews.llvm.org/D24816#558657 , @echristo wrote:
> 

> > > In https://reviews.llvm.org/D24816#550786 , @spatel wrote:
> 
> > >
> 
> > > > I wondered how that prerequisite check for fast-math was
> > > > currently handled:
> 
> > > >
> 
> > > > if (Options.UnsafeFPMath) {
> 
> > > >
> 
> > > >
> 
> > > > which gets set by:
> 
> > > >
> 
> > > > // FIXME: This function needs to go away for a number of
> > > > reasons:
> 
> > > > // a) global state on the TargetMachine is terrible in general,
> 
> > > > // b) there's no default state here to keep,
> 
> > > > // c) these target options should be passed only on the
> > > > function
> 
> > > > // and not on the TargetMachine (via TargetOptions) at all.
> 
> > > > void TargetMachine::resetTargetOptions(const Function &F) const
> > > > {
> 
> > > >
> 
> > > >
> 
> > > > ...which made me think of this bug:
> 
> > > > https://llvm.org/bugs/show_bug.cgi?id=23172
> 
> > > >
> 
> > > > The world is broken in a way much bigger than inlining too
> > > > strict
> > > > or too loose reciprocal estimate codegen settings, isn't it?
> 
> > >
> 
> > >
> 
> > > Pretty much. I've been trying to get all of the ones that
> > > "matter"
> > > out of there as fast as possible.
> 
> > >
> 
> > > That said, what -would- you like to do for inlining here? It
> > > seems
> > > like you're going to want a target routine there on matching. My
> > > guess for now is that you actually expect everything to be
> > > compiled with the same options and so not inlining based on
> > > difference would also be safe. @hfinkel thoughts here?
> 

> > I think we need to be careful here; users set this option, in some
> > cases, so that they can use fast-math but still get the accuracy
> > they require (by increasing the default number of Newton
> > iterations). We should really not inline functions that require
> > more
> > iterations into functions that require fewer. Obviously using
> > per-instruction metadata fixes this is a much nicer way. For better
> > or worse, this is really only an issue for LTO builds.
> 

> Sure, we can check for equal as a first check if we don't think it's
> going to come up much sometime soon
Makes sense to me. 

> (also, might want an attribute for this?).
You mean a C++ attribute? 

-Hal 

> -eric

> > > A couple of comments inline otherwise I'm fine with this
> > > implementation for now.
> 
> > >
> 
> > > Thanks!
> 
> > >
> 
> > > -eric
> 

> > https://reviews.llvm.org/D24816
> 

-- 

Hal Finkel 
Lead, Compiler Technology and Programming Languages 
Leadership Computing Facility 
Argonne National Laboratory 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161003/6a93c2c3/attachment.html>