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

[Sanjay Patel via llvm-commits]

spatel marked an inline comment as done.
spatel added inline comments.


> echristo wrote in TargetLowering.h:2178
> 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.



> hfinkel wrote in TargetRecip.h:51
> This TODO does not explain what this means. Do you mean using -1 RefinementSteps instead of having a separate boolean flag?
> 
> We probably should just do this, the current interface which has "true, Steps" everywhere is not aesthetically pleasing.

So yes, I agree this looks ugly, and it seemed simple enough to just combine the fields into a single int value when I added the TODO comment, but on closer inspection, it's not trivial because the user is allowed to specify "default" for the enablement part and still change the number of refinement steps. I think this also mucks with Eric's suggestions to simplify the 'set' method and/or just pull it all into the local users.

There's probably some relatively simple way to do this that I'm just not seeing right now, but even if there is, I'd rather not jeopardize the important thing (getting this out of target options) by introducing a bug while refactoring.

I've also convinced myself that I just need to get back on the FMF wagon and finish the DAG work and the clang/IR changes, so we can finally be clear of this mess. If that succeeds, then the ugly bits are going to disappear anyway. :)

https://reviews.llvm.org/D24816