guard fsqrt with fmf sub flags

[mcberg2017]

Summary

This change uses fmf subflags to guard optimizations as well as unsafe. These changes originated from D46483.
It contains only context for fsqrt.

[spatel]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

10897

Here, we're entering a bit of uncharted FMF territory. cc'ing some more potential reviewers for opinions - @wristow @efriedma @andrew.w.kaylor

I think we want to allow this transform when the node allows "approximate functions" (afn). I don't think we should care about 'arcp' - the transform of 1.0/sqrt(x) is handled on a different path AFAICT.

[mcberg2017]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

10897

Not quite true, I reference AMDGPU which uses 1.0/sqrt to map to AMDGPUISD::RSQ, which is rsqrt, although a partial condition. See SITargetLowering::lowerFastUnsafeFDIV. Adding Matt as this is a AMD topic.

[wristow]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

10897

It does seem to me that 'afn' is what we want here, rather than 'arcp'. I think the point about SITargetLowering::lowerFastUnsafeFDIV means that the code there ought to be checking both 'afn' and 'arcp'.

[arsenm]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

10897

The point of the code there is it doesn't need either. In some situations the instruction does the right thing without any need for a flag

[arsenm]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

10897

For the cases where it's not legal, I suppose checking both makes sense

[efriedma]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

10897

I can't see how arcp is relevant to this patch at all; visitFSQRT doesn't deal with division.

The flags required for rsqrt were discussed here: https://reviews.llvm.org/D37686#877987 .

[mcberg2017]

My take on Eli's point from D37686 is afn or unsafe is what we need then. The code and tests are augmented for that change.

[hfinkel]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

10897

I can't see how arcp is relevant to this patch at all; visitFSQRT doesn't deal with division.

I agree. This is just a question of how we compute sqrt(x). For an approximation of 1/sqrt(x), then I can see also needing arcp.

[spatel]

test/CodeGen/PowerPC/fmf-propagation.ll

368–370

Is there some reason for trimming the output here? I think it's important that we show the entire estimate sequence here to be consistent. Otherwise, it's misleading as it appears that we only need the raw estimate instruction.

[mcberg2017]

So should we include arcp in the check so that buildSqrtEstimate fires for arcp opportunities(to produce rsqrt) as well or is it fine the way it is?

[spatel]

In D47749#1123864, @mcberg2017 wrote:

So should we include arcp in the check so that buildSqrtEstimate fires for arcp opportunities(to produce rsqrt) as well or is it fine the way it is?

No - everyone agrees that this transform is predicated with 'afn' alone. The reciprocal sqrt transform is an independent problem/patch.

LGTM.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

SDValue DAGCombiner::visitFREM(SDNode *N) {
 
   if (SDValue NewSel = foldBinOpIntoSelect(N))
     return NewSel;
 
   return SDValue();
 }
 
 SDValue DAGCombiner::visitFSQRT(SDNode *N) {
-  if (!DAG.getTarget().Options.UnsafeFPMath)
+  SDNodeFlags Flags = N->getFlags();
+  if (!DAG.getTarget().Options.UnsafeFPMath && 
+      !Flags.hasApproximateFuncs())
     return SDValue();
 
   SDValue N0 = N->getOperand(0);
   if (TLI.isFsqrtCheap(N0, DAG))
     return SDValue();
 
-  // TODO: FSQRT nodes should have flags that propagate to the created nodes.
-  // For now, create a Flags object for use with reassociation math transforms.
-  SDNodeFlags Flags;
-  Flags.setAllowReassociation(true);
+  // FSQRT nodes have flags that propagate to the created nodes.
   return buildSqrtEstimate(N0, Flags);
 }
 
 /// copysign(x, fp_extend(y)) -> copysign(x, y)
 /// copysign(x, fp_round(y)) -> copysign(x, y)
 static inline bool CanCombineFCOPYSIGN_EXTEND_ROUND(SDNode *N) {
   SDValue N1 = N->getOperand(1);
   if ((N1.getOpcode() == ISD::FP_EXTEND ||

test/CodeGen/PowerPC/fmf-propagation.ll

; GLOBAL-NEXT:    blr
   %mul = fmul fast float %x, 42.0
   %fma = call fast float @llvm.fma.f32(float %x, float 7.0, float %mul)
   ret float %fma
 }
 
 ; Reduced precision for sqrt is allowed - should use estimate and NR iterations.
 
 ; FMFDEBUG-LABEL: Optimized lowered selection DAG: %bb.0 'sqrt_afn:'
-; FMFDEBUG:         fsqrt afn {{t[0-9]+}}
+; FMFDEBUG:         fmul afn {{t[0-9]+}}
 ; FMFDEBUG:       Type-legalized selection DAG: %bb.0 'sqrt_afn:'
 
 ; GLOBALDEBUG-LABEL: Optimized lowered selection DAG: %bb.0 'sqrt_afn:'
-; GLOBALDEBUG:         fmul reassoc {{t[0-9]+}}
+; GLOBALDEBUG:         fmul afn {{t[0-9]+}}
 ; GLOBALDEBUG:       Type-legalized selection DAG: %bb.0 'sqrt_afn:'
 
 define float @sqrt_afn(float %x) {
 ; FMF-LABEL: sqrt_afn:
-; FMF:       # %bb.0:
-; FMF-NEXT:    xssqrtsp 1, 1
-; FMF-NEXT:    blr
+; FMF:          # %bb.0:
+; FMF-NEXT:       xxlxor 0, 0, 0
+; FMF-NEXT:       fcmpu 0, 1, 0
+; FMF-NEXT:       beq 0, .LBB10_2
+; FMF-NEXT:     # %bb.1:
+; FMF-NEXT:       addis 3, 2, .LCPI10_0@toc@ha
+; FMF-NEXT:       xsrsqrtesp 3, 1
+; FMF-NEXT:       addi 3, 3, .LCPI10_0@toc@l
+; FMF-NEXT:       lfsx 0, 0, 3
+; FMF-NEXT:       xsmulsp 2, 1, 0
+; FMF-NEXT:       xsmulsp 4, 3, 3
+; FMF-NEXT:       xssubsp 2, 2, 1
+; FMF-NEXT:       xsmulsp 2, 2, 4
+; FMF-NEXT:       xssubsp 0, 0, 2
+; FMF-NEXT:       xsmulsp 0, 3, 0
+; FMF-NEXT:       xsmulsp 0, 0, 1
+; FMF-NEXT:     .LBB10_2:
+; FMF-NEXT:       fmr 1, 0
+; FMF-NEXT:       blr
 ;
 ; GLOBAL-LABEL: sqrt_afn:
 ; GLOBAL:       # %bb.0:
 ; GLOBAL-NEXT:    xxlxor 0, 0, 0
 ; GLOBAL-NEXT:    fcmpu 0, 1, 0
 ; GLOBAL-NEXT:    beq 0, .LBB10_2
 ; GLOBAL-NEXT:  # %bb.1:
 ; GLOBAL-NEXT:    xsrsqrtesp 2, 1

 ; GLOBAL-NEXT:    blr
   %rt = call afn float @llvm.sqrt.f32(float %x)
   ret float %rt
 }
 
 ; The call is now fully 'fast'. This implies that approximation is allowed.
 
 ; FMFDEBUG-LABEL: Optimized lowered selection DAG: %bb.0 'sqrt_fast:'
-; FMFDEBUG:         fsqrt nnan ninf nsz arcp contract afn reassoc {{t[0-9]+}}
+; FMFDEBUG:         fmul nnan ninf nsz arcp contract afn reassoc {{t[0-9]+}}
 ; FMFDEBUG:       Type-legalized selection DAG: %bb.0 'sqrt_fast:'
 
 ; GLOBALDEBUG-LABEL: Optimized lowered selection DAG: %bb.0 'sqrt_fast:'
-; GLOBALDEBUG:         fmul reassoc {{t[0-9]+}}
+; GLOBALDEBUG:         fmul nnan ninf nsz arcp contract afn reassoc {{t[0-9]+}}
 ; GLOBALDEBUG:       Type-legalized selection DAG: %bb.0 'sqrt_fast:'
 
 define float @sqrt_fast(float %x) {
 ; FMF-LABEL: sqrt_fast:
-; FMF:       # %bb.0:
-; FMF-NEXT:    xssqrtsp 1, 1
-; FMF-NEXT:    blr
+; FMF:       # %bb.1:
+; FMF-NEXT:    xsrsqrtesp 2, 1
+; FMF:         blr
 ;
 ; GLOBAL-LABEL: sqrt_fast:
 ; GLOBAL:       # %bb.0:
 ; GLOBAL-NEXT:    xxlxor 0, 0, 0
 ; GLOBAL-NEXT:    fcmpu 0, 1, 0
 ; GLOBAL-NEXT:    beq 0, .LBB11_2
 ; GLOBAL-NEXT:  # %bb.1:
 ; GLOBAL-NEXT:    xsrsqrtesp 2, 1

test/CodeGen/X86/fmf-flags.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=x86_64-unknown | FileCheck %s -check-prefix=X64
 ; RUN: llc < %s -mtriple=i686-unknown   | FileCheck %s -check-prefix=X86
 
 declare float @llvm.sqrt.f32(float %x);
 
 define float @fast_recip_sqrt(float %x) {
 ; X64-LABEL: fast_recip_sqrt:
 ; X64:       # %bb.0:
-; X64-NEXT:    sqrtss %xmm0, %xmm1
-; X64-NEXT:    movss {{.*#+}} xmm0 = mem[0],zero,zero,zero
-; X64-NEXT:    divss %xmm1, %xmm0
+; X64-NEXT:    rsqrtss %xmm0, %xmm1
+; X64-NEXT:    xorps   %xmm2, %xmm2
+; X64-NEXT:    cmpeqss %xmm0, %xmm2
+; X64-NEXT:    mulss   %xmm1, %xmm0
+; X64-NEXT:    movss   {{.*}}(%rip), %xmm3
+; X64-NEXT:    mulss   %xmm0, %xmm3
+; X64-NEXT:    mulss   %xmm1, %xmm0
+; X64-NEXT:    addss   {{.*}}(%rip), %xmm0
+; X64-NEXT:    mulss   %xmm3, %xmm0
+; X64-NEXT:    andnps  %xmm0, %xmm2
+; X64-NEXT:    movss   {{.*}}(%rip), %xmm0
+; X64-NEXT:    divss   %xmm2, %xmm0
 ; X64-NEXT:    retq
 ;
 ; X86-LABEL: fast_recip_sqrt:
 ; X86:       # %bb.0:
 ; X86-NEXT:    flds {{[0-9]+}}(%esp)
 ; X86-NEXT:    fsqrt
 ; X86-NEXT:    fld1
 ; X86-NEXT:    fdivp %st(1)

test/CodeGen/X86/sqrt-fastmath-mir.ll

 ; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=avx2,fma -stop-after=expand-isel-pseudos 2>&1 | FileCheck %s
 
 declare float @llvm.sqrt.f32(float) #0
 
 define float @foo(float %f) #0 {
 ; CHECK: {{name: *foo}}
 ; CHECK: body:
 ; CHECK:     %0:fr32 = COPY $xmm0
 ; CHECK:     %1:fr32 = VRSQRTSSr killed %2, %0
-; CHECK:     %3:fr32 = reassoc VMULSSrr %0, %1
+; CHECK:     %3:fr32 = VMULSSrr %0, %1
 ; CHECK:     %4:fr32 = VMOVSSrm
 ; CHECK:     %5:fr32 = VFMADD213SSr %1, killed %3, %4
 ; CHECK:     %6:fr32 = VMOVSSrm
-; CHECK:     %7:fr32 = reassoc VMULSSrr %1, %6
-; CHECK:     %8:fr32 = reassoc VMULSSrr killed %7, killed %5
-; CHECK:     %9:fr32 = reassoc VMULSSrr %0, %8
+; CHECK:     %7:fr32 = VMULSSrr %1, %6
+; CHECK:     %8:fr32 = VMULSSrr killed %7, killed %5
+; CHECK:     %9:fr32 = VMULSSrr %0, %8
 ; CHECK:     %10:fr32 = VFMADD213SSr %8, %9, %4
-; CHECK:     %11:fr32 = reassoc VMULSSrr %9, %6
-; CHECK:     %12:fr32 = reassoc VMULSSrr killed %11, killed %10
+; CHECK:     %11:fr32 = VMULSSrr %9, %6
+; CHECK:     %12:fr32 = VMULSSrr killed %11, killed %10
 ; CHECK:     %14:fr32 = FsFLD0SS
 ; CHECK:     %15:fr32 = VCMPSSrr %0, killed %14, 0
 ; CHECK:     %17:vr128 = VANDNPSrr killed %16, killed %13
 ; CHECK:     $xmm0 = COPY %18
 ; CHECK:     RET 0, $xmm0
   %call = tail call float @llvm.sqrt.f32(float %f) #1
   ret float %call
 }