[X86][FP16] Only generate approximate rsqrt when Reciprocal is true for half type

[pengfei]

Summary

We have reasonable fast sqrt and accurate rsqrt for half type due to the
limited fractions. So neither do we need multi steps refinement for
rsqrt nor replace sqrt by rsqrt.
This fixes a correctness issue when RefinementSteps = 0.

[craig.topper]

What's the correctness issue? It's fast math so the answer isn't required to be exact.

[pengfei]

@kmclaughlin I'm meant to change in the target independent code (enabled in D110557), then I found the test in your D111657 was affected too. I'll change there if you think it is workable for aarch64 too.

[pengfei]

In D114765#3160232, @craig.topper wrote:

What's the correctness issue? It's fast math so the answer isn't required to be exact.

See https://godbolt.org/z/P4Y89hsj4
I guess we need at least RefinementSteps = 1 for correntness when Reciprocal == 0.

[craig.topper]

In D114765#3160245, @pengfei wrote:

In D114765#3160232, @craig.topper wrote:

What's the correctness issue? It's fast math so the answer isn't required to be exact.

See https://godbolt.org/z/P4Y89hsj4
I guess we need at least RefinementSteps = 1 for correntness when Reciprocal == 0.

That seems like a more general bug. The same issue happens if you force the estimate steps for floating point https://godbolt.org/z/vK8eab8zP

[craig.topper]

I think for f16 you should also be returning true from X86TargetLowering::isFsqrtCheap which would prevent getSqrtEstimate from being called for the non-reciprocal case.

[pengfei]

Return false for f16 in X86TargetLowering::isFsqrtCheap.

[pengfei]

In D114765#3160256, @craig.topper wrote:

In D114765#3160245, @pengfei wrote:

In D114765#3160232, @craig.topper wrote:

What's the correctness issue? It's fast math so the answer isn't required to be exact.

See https://godbolt.org/z/P4Y89hsj4
I guess we need at least RefinementSteps = 1 for correntness when Reciprocal == 0.

That seems like a more general bug. The same issue happens if you force the estimate steps for floating point https://godbolt.org/z/vK8eab8zP

I see. But I'm not sure if it is bug or by design. Maybe need an assertion?

In D114765#3160268, @craig.topper wrote:

I think for f16 you should also be returning true from X86TargetLowering::isFsqrtCheap which would prevent getSqrtEstimate from being called for the non-reciprocal case.

Good point!

[craig.topper]

In D114765#3160387, @pengfei wrote:

In D114765#3160256, @craig.topper wrote:

In D114765#3160245, @pengfei wrote:

In D114765#3160232, @craig.topper wrote:

What's the correctness issue? It's fast math so the answer isn't required to be exact.

See https://godbolt.org/z/P4Y89hsj4
I guess we need at least RefinementSteps = 1 for correntness when Reciprocal == 0.

That seems like a more general bug. The same issue happens if you force the estimate steps for floating point https://godbolt.org/z/vK8eab8zP

I see. But I'm not sure if it is bug or by design. Maybe need an assertion?

I think this is a weird interface quirk. The caller interprets returning RefinementSteps = 0 to mean that all needed code has been created and nothing should be done. Theoretically a target could have its own way of handling it without the final FMUL the target independent code inserts. So I think X86 should either insert the final FMUL itself or not do the reciprocal approximation for non-reciprocal if RefinementSteps is 0. NVPTXTargetLowering::getSqrtEstimate does the latter.

[pengfei]

So I think X86 should either insert the final FMUL itself or ...

Insert a FMUL. Thanks Craig!

[spatel]

In D114765#3160396, @craig.topper wrote:

In D114765#3160387, @pengfei wrote:

In D114765#3160256, @craig.topper wrote:

In D114765#3160245, @pengfei wrote:

In D114765#3160232, @craig.topper wrote:

What's the correctness issue? It's fast math so the answer isn't required to be exact.

See https://godbolt.org/z/P4Y89hsj4
I guess we need at least RefinementSteps = 1 for correntness when Reciprocal == 0.

That seems like a more general bug. The same issue happens if you force the estimate steps for floating point https://godbolt.org/z/vK8eab8zP

I see. But I'm not sure if it is bug or by design. Maybe need an assertion?

I think this is a weird interface quirk. The caller interprets returning RefinementSteps = 0 to mean that all needed code has been created and nothing should be done. Theoretically a target could have its own way of handling it without the final FMUL the target independent code inserts. So I think X86 should either insert the final FMUL itself or not do the reciprocal approximation for non-reciprocal if RefinementSteps is 0. NVPTXTargetLowering::getSqrtEstimate does the latter.

That looks like a long-standing bug for x86. I don't remember if that was a design flaw originally or if the code evolved to handle more patterns, and we just missed that problem.
Better to add the tests first, add the code to fix the bug, then add the code to bypass for f16?

llvm/test/CodeGen/X86/avx512fp16vl-intrinsics.ll

972

Does it make sense to add a scalar test too?

define half @sqrt_half_fast(half %a0, half %a1) {
  %1 = call fast half @llvm.sqrt.f16(half %a0)
  ret half %1
}

[pengfei]

Break it into 3 patches: rG65a3de91ab3e, D114843 and D114844.

llvm/lib/Target/X86/X86ISelLowering.cpp

static SDValue EmitCmp(SDValue Op0, SDValue Op1, unsigned X86CC,
   SDValue Sub = DAG.getNode(X86ISD::SUB, dl, VTs, Op0, Op1);
   return Sub.getValue(1);
 }
 
 /// Check if replacement of SQRT with RSQRT should be disabled.
 bool X86TargetLowering::isFsqrtCheap(SDValue Op, SelectionDAG &DAG) const {
   EVT VT = Op.getValueType();
 
+  // We don't need to replace SQRT with RSQRT for half type.
+  if (VT.getScalarType() == MVT::f16)
+    return true;
+
   // We never want to use both SQRT and RSQRT instructions for the same input.
   if (DAG.getNodeIfExists(X86ISD::FRSQRT, DAG.getVTList(VT), Op))
     return false;
 
   if (VT.isVector())
     return Subtarget.hasFastVectorFSQRT();
   return Subtarget.hasFastScalarFSQRT();
 }
if ((VT == MVT::f32 && Subtarget.hasSSE1()) ||
     UseOneConstNR = false;
     // There is no FSQRT for 512-bits, but there is RSQRT14.
     unsigned Opcode = VT == MVT::v16f32 ? X86ISD::RSQRT14 : X86ISD::FRSQRT;
     return DAG.getNode(Opcode, DL, VT, Op);
   }
 
   if (VT.getScalarType() == MVT::f16 && isTypeLegal(VT) &&
       Subtarget.hasFP16()) {
+    assert(Reciprocal && "Don't replace SQRT with RSQRT for half type");
     if (RefinementSteps == ReciprocalEstimate::Unspecified)
       RefinementSteps = 0;
 
     if (VT == MVT::f16) {
       SDValue Zero = DAG.getIntPtrConstant(0, DL);
       SDValue Undef = DAG.getUNDEF(MVT::v8f16);
       Op = DAG.getNode(ISD::SCALAR_TO_VECTOR, DL, MVT::v8f16, Op);
       Op = DAG.getNode(X86ISD::RSQRT14S, DL, MVT::v8f16, Undef, Op);

llvm/test/CodeGen/X86/avx512fp16vl-intrinsics.ll

 ; CHECK-NEXT:    vrsqrtph %xmm0, %xmm0
 ; CHECK-NEXT:    vmulph %xmm0, %xmm1, %xmm0
 ; CHECK-NEXT:    retq
   %1 = call fast <8 x half> @llvm.sqrt.v8f16(<8 x half> %a0)
   %2 = fdiv fast <8 x half> %a1, %1
   ret <8 x half> %2
 }
 
+define <8 x half> @test_sqrt_ph_128_fast2(<8 x half> %a0, <8 x half> %a1) {
+; CHECK-LABEL: test_sqrt_ph_128_fast2:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    vsqrtph %xmm0, %xmm0
+; CHECK-NEXT:    retq
+  %1 = call fast <8 x half> @llvm.sqrt.v8f16(<8 x half> %a0)
+  ret <8 x half> %1
+}
+
 define <8 x half> @test_mask_sqrt_ph_128(<8 x half> %a0, <8 x half> %passthru, i8 %mask) {
 ; CHECK-LABEL: test_mask_sqrt_ph_128:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    kmovd %edi, %k1
 ; CHECK-NEXT:    vsqrtph %xmm0, %xmm1 {%k1}
 ; CHECK-NEXT:    vmovaps %xmm1, %xmm0
 ; CHECK-NEXT:    retq
   %1 = call <8 x half> @llvm.sqrt.v8f16(<8 x half> %a0)