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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

This looks good, although please add a test case for sqrtf() where we've specific a non-zero number of extra steps. I ask because, unfortunately, I think it won't work correctly. The code in DAGCombiner::buildSqrtEstimateImpl, when some number of extra iterations are requested, calls buildSqrtNROneConst (or buildSqrtNRTwoConst), and these seem to assume that the incoming base estimate is always a reciprocal estimate, and so they always need to multiply by x so that they compute sqrt(x) = x*rsqrt(x).

To refine an estimate for sqrt, instead of using the iteration `X_{i+1} = X_i (1.5 - A X_i^2 / 2)`, you'd use `X_{i+1} = 0.5(X_i + A/X_i)`. The problem here is that you need to divide to make this work, and that defeats the purpose of having a fast approximation. As a result, the apparently "hacky" solution here is actually the best: When extra iterations are requested, the function should return the rsqrt estimate instead of the sqrt estimate. I think we just need to clean up the interface a bit to make all of this really make sense. This seems to indicate that, if I'm reading the code correctly, we have a bug here for other backends for sqrt() when no extra iterations are requested (it will directly return the rsqrt estimate instead of the proper sqrt estimate). The fact that we might return a sqrt estimate directly, when no iterations are requested, seems like a special case. Maybe the best thing is to always return rsqrt here, but when no iterations are requested for sqrt, just return SDValue() and match the sqrt later in TableGen?


https://reviews.llvm.org/D28508