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

Hal Finkel via llvm-commits llvm-commits at lists.llvm.org
Sun Oct 2 19:27:29 PDT 2016


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.

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




https://reviews.llvm.org/D24816





More information about the llvm-commits mailing list