[PATCH] D153472: AMDGPU: Correctly expand f64 sqrt intrinsic

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In D153472#4538112 <https://reviews.llvm.org/D153472#4538112>, @foad wrote:

>> I am tempted to do this in an IR expansion instead. In the IR
>> we could take advantage of computeKnownFPClass to avoid
>> the 0-or-inf argument check.
>
> Could you use computeKnownFPClass to set ninf and nsz if the argument is known not to be inf/0, and then check those flags in your existing legalization code?



In D153472#4538112 <https://reviews.llvm.org/D153472#4538112>, @foad wrote:

>> I am tempted to do this in an IR expansion instead. In the IR
>> we could take advantage of computeKnownFPClass to avoid
>> the 0-or-inf argument check.
>
> Could you use computeKnownFPClass to set ninf and nsz if the argument is known not to be inf/0, and then check those flags in your existing legalization code?

Yes, in this case it seems feasible. We don't in general infer fast math flags anywhere. It's usually difficult because they constrain both inputs and outputs



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:4861
+  //
+  //   sqrt(x) = g3
+
----------------
foad wrote:
> It's nice that you have a comment showing the algorithm, but it'd be way more useful if the names in the comment matched the names in the code.
> 
> Also what do the `=>` mean? It looks like they show alternative expansions, and I guess we implement the versions on the RHS?? I can see that the second one avoids the need to calculate h2, but what is the point of the first one?
Copy paste, this is ported code


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:4873
+
+  auto ScaleConstant = B.buildFConstant(F64, 0x1.0p-767);
+
----------------
foad wrote:
> What is this magic number and why is scaling required? If it is to avoid denormal inputs to amdgcn_rsq then please say that in a comment.
Your guess is as good as mine, this is all ported and none of this stuff was ever well documented


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153472/new/

https://reviews.llvm.org/D153472