[DAGCombiner] Add nsz constraint to aggressive fma folding

[qiucf]

Summary

There's a folding pattern in DAGCombine:

(fsub x, (fma y, z, (fmul u, v)))
->
(fma (fneg y), z, (fma (fneg u), v, x))

However, since -0-0=-0; -0+0=+0, if yz=1, uv=-1, x=-0:

x-(yz+uv) = -0
-yz+(-uv+x) = +0

So this requires nsz.

[spatel]

llvm/test/CodeGen/AMDGPU/fma-combine.ll

3

It would be better to fix this RUN line (or better: add IR-level FMF to the changed test).

llvm/test/CodeGen/AMDGPU/mad-combine.ll

5

It would be better to fix this RUN line (or better: add IR-level FMF to the changed test).

llvm/test/CodeGen/PowerPC/fma-assoc.ll

102–105

Please create a test that uses IR-level FMF ("reassoc nsz" or "fast"?) entirely. If you can update this file to avoid the global "-enable-unsafe-fp-math" as a preliminary commit, that would be best.

It would also be great to use "utils/update_llc_test_checks.py" to auto-generate complete CHECK lines as a preliminary NFC commit.

[qiucf]

Update some test cases

[spatel]

LGTM

llvm/test/CodeGen/PowerPC/fma-assoc.ll

102–105

That's not what I was hoping for, but I see now that this whole test file should be updated. We should treat "-enable-unsafe-fp-math" on the RUN line as deprecated and replace that with "fast" or "reassoc" in the IR. But that will require duplicating tests if I'm seeing it correctly.

I won't block this correctness fix on the test improvements, but please make that change as an NFC follow-up or least mark this file with a FIXME.

[qiucf]

llvm/test/CodeGen/PowerPC/fma-assoc.ll

102–105

Thanks for explanation. I committed rGc1bc56bf4f and rG996dc13dc first and then this one. BTW, options/flags for FP operations may sometimes be confusing. It seems they should be clarified/reorganized.

[spatel]

llvm/test/CodeGen/PowerPC/fma-assoc.ll

102–105

Thanks for cleaning that test file! If you have a suggestion for improving the docs/comments for these flags, then we can fix it. I agree that it is messy.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

SDValue DAGCombiner::visitFSUBForFMACombine(SDNode *N) {
 
   const SelectionDAGTargetInfo *STI = DAG.getSubtarget().getSelectionDAGInfo();
   if (STI && STI->generateFMAsInMachineCombiner(OptLevel))
     return SDValue();
 
   // Always prefer FMAD to FMA for precision.
   unsigned PreferredFusedOpcode = HasFMAD ? ISD::FMAD : ISD::FMA;
   bool Aggressive = TLI.enableAggressiveFMAFusion(VT);
+  bool NoSignedZero = Options.NoSignedZerosFPMath || Flags.hasNoSignedZeros();
 
   // Is the node an FMUL and contractable either due to global flags or
   // SDNodeFlags.
   auto isContractableFMUL = [AllowFusionGlobally](SDValue N) {
     if (N.getOpcode() != ISD::FMUL)
       return false;
     return AllowFusionGlobally || isContractable(N.getNode());
   };
if (CanFuse && N0.getOpcode() == PreferredFusedOpcode &&
                                      DAG.getNode(ISD::FNEG, SL, VT,
                                                  N1), Flags), Flags);
     }
 
     // fold (fsub x, (fma y, z, (fmul u, v)))
     //   -> (fma (fneg y), z, (fma (fneg u), v, x))
     if (CanFuse && N1.getOpcode() == PreferredFusedOpcode &&
         isContractableFMUL(N1.getOperand(2)) &&
-        N1->hasOneUse()) {
+        N1->hasOneUse() && NoSignedZero) {
       SDValue N20 = N1.getOperand(2).getOperand(0);
       SDValue N21 = N1.getOperand(2).getOperand(1);
       return DAG.getNode(PreferredFusedOpcode, SL, VT,
                          DAG.getNode(ISD::FNEG, SL, VT,
                                      N1.getOperand(0)),
                          N1.getOperand(1),
                          DAG.getNode(PreferredFusedOpcode, SL, VT,
                                      DAG.getNode(ISD::FNEG, SL, VT, N20),

llvm/test/CodeGen/AMDGPU/fma-combine.ll

 ; RUN:  llc -amdgpu-scalarize-global-loads=false  -march=amdgcn -mcpu=tahiti -verify-machineinstrs -fp-contract=fast < %s | FileCheck -enable-var-scope -check-prefix=SI-NOFMA -check-prefix=SI-SAFE -check-prefix=SI -check-prefix=FUNC %s
 ; RUN:  llc -amdgpu-scalarize-global-loads=false  -march=amdgcn -mcpu=verde -verify-machineinstrs -fp-contract=fast < %s | FileCheck -enable-var-scope -check-prefix=SI-NOFMA -check-prefix=SI-SAFE -check-prefix=SI -check-prefix=FUNC %s
 ; RUN:  llc -amdgpu-scalarize-global-loads=false  -march=amdgcn -mcpu=tahiti -verify-machineinstrs -fp-contract=fast -enable-no-infs-fp-math -enable-unsafe-fp-math -mattr=+fp32-denormals < %s | FileCheck -enable-var-scope -check-prefix=SI-FMA -check-prefix=SI-UNSAFE -check-prefix=SI -check-prefix=FUNC %s
 
+; FIXME: Remove enable-unsafe-fp-math in RUN line and add flags to IR instrs
+
 ; Note: The SI-FMA conversions of type x * (y + 1) --> x * y + x would be
 ; beneficial even without fp32 denormals, but they do require no-infs-fp-math
 ; for correctness.
 
 declare i32 @llvm.amdgcn.workitem.id.x() #0
 declare double @llvm.fabs.f64(double) #0
 declare double @llvm.fma.f64(double, double, double) #0
 declare float @llvm.fma.f32(float, float, float) #0
define amdgpu_kernel void @aggressive_combine_to_fma_fsub_1_f64(double addrspace(1)* noalias %out, double addrspace(1)* noalias %in) #1 {
   %gep.out = getelementptr double, double addrspace(1)* %out, i32 %tid
 
   %x = load volatile double, double addrspace(1)* %gep.0
   %y = load volatile double, double addrspace(1)* %gep.1
   %z = load volatile double, double addrspace(1)* %gep.2
   %u = load volatile double, double addrspace(1)* %gep.3
   %v = load volatile double, double addrspace(1)* %gep.4
 
-  %tmp0 = fmul double %u, %v
-  %tmp1 = call double @llvm.fma.f64(double %y, double %z, double %tmp0) #0
-  %tmp2 = fsub double %x, %tmp1
+  ; nsz flag is needed since this combine may change sign of zero
+  %tmp0 = fmul nsz double %u, %v
+  %tmp1 = call nsz double @llvm.fma.f64(double %y, double %z, double %tmp0) #0
+  %tmp2 = fsub nsz double %x, %tmp1
 
   store double %tmp2, double addrspace(1)* %gep.out
   ret void
 }
 
 ;
 ; Patterns (+ fneg variants): mul(add(1.0,x),y), mul(sub(1.0,x),y), mul(sub(x,1.0),y)
 ;

llvm/test/CodeGen/AMDGPU/mad-combine.ll

 ; Make sure we still form mad even when unsafe math or fp-contract is allowed instead of fma.
 
 ; RUN: llc -march=amdgcn -mcpu=tahiti -verify-machineinstrs < %s | FileCheck -enable-var-scope -check-prefix=SI -check-prefix=SI-STD  -check-prefix=SI-STD-SAFE -check-prefix=FUNC %s
 ; RUN: llc -march=amdgcn -mcpu=tahiti -verify-machineinstrs -fp-contract=fast < %s | FileCheck -enable-var-scope -check-prefix=SI -check-prefix=SI-STD -check-prefix=SI-STD-SAFE -check-prefix=FUNC %s
 ; RUN: llc -march=amdgcn -mcpu=tahiti -verify-machineinstrs -enable-unsafe-fp-math < %s | FileCheck -enable-var-scope -check-prefix=SI -check-prefix=SI-STD -check-prefix=SI-STD-UNSAFE -check-prefix=FUNC %s
 
+; FIXME: Remove enable-unsafe-fp-math in RUN line and add flags to IR instrs
+
 ; Make sure we don't form mad with denormals
 ; RUN: llc -march=amdgcn -mcpu=tahiti -mattr=+fp32-denormals -fp-contract=fast -verify-machineinstrs < %s | FileCheck -enable-var-scope -check-prefix=SI -check-prefix=SI-DENORM -check-prefix=SI-DENORM-FASTFMAF -check-prefix=FUNC %s
 ; RUN: llc -march=amdgcn -mcpu=verde -mattr=+fp32-denormals -fp-contract=fast -verify-machineinstrs < %s | FileCheck -enable-var-scope -check-prefix=SI -check-prefix=SI-DENORM -check-prefix=SI-DENORM-SLOWFMAF -check-prefix=FUNC %s
 
 declare i32 @llvm.amdgcn.workitem.id.x() #0
 declare float @llvm.fabs.f32(float) #0
 declare float @llvm.fma.f32(float, float, float) #0
 declare float @llvm.fmuladd.f32(float, float, float) #0
define amdgpu_kernel void @aggressive_combine_to_mad_fsub_3_f32(float addrspace(1)* noalias %out, float addrspace(1)* noalias %in) #1 {
   %gep.out = getelementptr float, float addrspace(1)* %out, i32 %tid
 
   %x = load volatile float, float addrspace(1)* %gep.0
   %y = load volatile float, float addrspace(1)* %gep.1
   %z = load volatile float, float addrspace(1)* %gep.2
   %u = load volatile float, float addrspace(1)* %gep.3
   %v = load volatile float, float addrspace(1)* %gep.4
 
-  %tmp0 = fmul float %u, %v
-  %tmp1 = call float @llvm.fmuladd.f32(float %y, float %z, float %tmp0) #0
-  %tmp2 = fsub float %x, %tmp1
+  ; nsz flag is needed since this combine may change sign of zero
+  %tmp0 = fmul nsz float %u, %v
+  %tmp1 = call nsz float @llvm.fmuladd.f32(float %y, float %z, float %tmp0) #0
+  %tmp2 = fsub nsz float %x, %tmp1
 
   store float %tmp2, float addrspace(1)* %gep.out
   ret void
 }
 
 attributes #0 = { nounwind readnone }
 attributes #1 = { nounwind }

llvm/test/CodeGen/PowerPC/fma-assoc.ll

 }
 
 define double @test_FMADD_ASSOC_EXT1(float %A, float %B, double %C,
 ; CHECK-LABEL: test_FMADD_ASSOC_EXT1:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    fmadd 0, 1, 2, 5
 ; CHECK-NEXT:    fmadd 1, 3, 4, 0
 ; CHECK-NEXT:    blr
 ;
 ; CHECK-VSX-LABEL: test_FMADD_ASSOC_EXT1:
 ; CHECK-VSX:       # %bb.0:
 ; CHECK-VSX-NEXT:    xsmaddmdp 1, 2, 5
 ; CHECK-VSX-NEXT:    xsmaddadp 1, 3, 4
 ; CHECK-VSX-NEXT:    blr
                                  double %D, double %E) {
   %F = fmul float %A, %B         ; <float> [#uses=1]
   %G = fpext float %F to double   ; <double> [#uses=1]
   %H = fmul double %C, %D         ; <double> [#uses=1]
   %I = fadd double %H, %G         ; <double> [#uses=1]
   %J = fadd double %I, %E         ; <double> [#uses=1]
; CHECK-VSX-NEXT:    blr
   %H = fadd reassoc double %F, %G         ; <double> [#uses=1]
   %I = fsub reassoc double %H, %E         ; <double> [#uses=1]
   ret double %I
 }
 
 define double @test_reassoc_FMSUB_ASSOC2(double %A, double %B, double %C,
 ; CHECK-LABEL: test_reassoc_FMSUB_ASSOC2:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    fnmsub 0, 3, 4, 5
-; CHECK-NEXT:    fnmsub 1, 1, 2, 0
+; CHECK-NEXT:    fmul 0, 3, 4
+; CHECK-NEXT:    fmadd 0, 1, 2, 0
+; CHECK-NEXT:    fsub 1, 5, 0
 ; CHECK-NEXT:    blr
 ;
 ; CHECK-VSX-LABEL: test_reassoc_FMSUB_ASSOC2:
 ; CHECK-VSX:       # %bb.0:
-; CHECK-VSX-NEXT:    xsnmsubmdp 3, 4, 5
-; CHECK-VSX-NEXT:    xsnmsubadp 3, 1, 2
-; CHECK-VSX-NEXT:    fmr 1, 3
+; CHECK-VSX-NEXT:    xsmuldp 0, 3, 4
+; CHECK-VSX-NEXT:    xsmaddadp 0, 1, 2
+; CHECK-VSX-NEXT:    xssubdp 1, 5, 0
 ; CHECK-VSX-NEXT:    blr
                                  double %D, double %E) {
   %F = fmul reassoc double %A, %B         ; <double> [#uses=1]
   %G = fmul reassoc double %C, %D         ; <double> [#uses=1]
   %H = fadd reassoc double %F, %G         ; <double> [#uses=1]
   %I = fsub reassoc double %E, %H         ; <double> [#uses=1]
   ret double %I
 }