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

Hal Finkel via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jan 30 19:54:15 PST 2017

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!


More information about the llvm-commits mailing list