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

[Eric Christopher via llvm-commits]

echristo added a comment.

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?

A couple of comments inline otherwise I'm fine with this implementation for now.

Thanks!

-eric



> TargetLowering.h:2178
>                                      MachineBasicBlock *MBB) const;
> +  TargetRecip ReciprocalEstimates;
>  };

Seems reasonable - otherwise possible to keep it local in the various routines that want to know about the estimates? Parse there rather than initialize at the beginning? That way the function above that builds up the struct can be the whole interface rather than caching. That said, not sure how often it's called.

> TargetLoweringBase.cpp:1488
> +  const Function *F = MF.getFunction();
> +  if (!F->hasFnAttribute("mrecip"))
> +    return ReciprocalEstimates;

Bikeshed: mrecip is pretty hard to understand (for me at least) naming wise, reciprocal-estimates while longer might be a bit more comprehensible?

https://reviews.llvm.org/D24816