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

[Eric Christopher via llvm-commits]

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 (also, might want an attribute for this?).

-eric


> > A couple of comments inline otherwise I'm fine with this implementation
> for now.
> >
> > Thanks!
> >
> > -eric
>
>
>
>
> https://reviews.llvm.org/D24816
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161003/3dd72fe5/attachment.html>