[PATCH] D28508: [NVPTX] Implement NVPTXTargetLowering::getSqrtEstimate.

[Hal Finkel via Phabricator via llvm-commits]

hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D28508#661279, @jlebar wrote:

> In https://reviews.llvm.org/D28508#661259, @hfinkel wrote:
>
> > The most natural way I can think of to fix this API, is the make it always expect a rsqrt estimate (including in the ExtraIterations == 0 case). In that case, if a target has a direct sqrt estimate to use in the ExtraIterations == 0 case, it would return SDValue() and then match the sqrt node in TableGen (or in C++). That fix is incompatible with the code here, so I'd like your opinion on this.
>
>
> I wonder if part of the problem is that the API is just too flexible.  In particular, what if we didn't let you modify NumIters within this function?
>
> We would have three or four TLI functions:
>
> - "get me the default number of Newton's method iters for an (r)sqrt approximation" (could be one method with a bool param or two; I lean towards two because http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html)
> - "get me an approximate rsqrt"
> - "get me an approximate sqrt", where the default returns x * approx_rsqrt(x).
>
>   The problem right now is that because the function may modify NumIters, everything has to be stuffed into one big, confusing function.
>
>   We don't strictly need "get me an approx sqrt", but defining it this way lets us have a reasonable default, which seems nice.
>
>   (I would still rather check in this patch and refactor separately, though.)


I think this makes sense. Let's commit this and then refactor/fix things. Thanks!


https://reviews.llvm.org/D28508