[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

133–136

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

133–136

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

133–136

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

133–136

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

 
   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());
   };

                                      N0.getOperand(2).getOperand(1),
                                      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),
                                      N21, N0, Flags), Flags);
     }
 
 
     // fold (fsub (fma x, y, (fpext (fmul u, v))), z)
     //   -> (fma x, y (fma (fpext u), (fpext v), (fneg z)))
     if (N0.getOpcode() == PreferredFusedOpcode &&
         N0->hasOneUse()) {
       SDValue N02 = N0.getOperand(2);
       if (N02.getOpcode() == ISD::FP_EXTEND) {
         SDValue N020 = N02.getOperand(0);
         if (isContractableFMUL(N020) &&

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
 
 ; 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
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
 
 ; 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
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_FMSUB_ASSOC2(double %A, double %B, double %C,
   ret double %I
 ; CHECK-SAFE-LABEL: test_FMSUB_ASSOC2:
 ; CHECK-SAFE: fmul
 ; CHECK-SAFE-NEXT: fmadd
 ; CHECK-SAFE-NEXT: fsub
 ; CHECK-SAFE-NEXT: blr
 
 ; CHECK-UNSAFE-LABEL: test_FMSUB_ASSOC2:
-; CHECK-UNSAFE: fnmsub
-; CHECK-UNSAFE-NEXT: fnmsub
+; CHECK-UNSAFE: fmul
+; CHECK-UNSAFE-NEXT: fmadd
+; CHECK-UNSAFE-NEXT: fsub
 ; CHECK-UNSAFE-NEXT: blr
 
 ; CHECK-SAFE-VSX-LABEL: test_FMSUB_ASSOC2:
 ; CHECK-SAFE-VSX: xsmuldp
 ; CHECK-SAFE-VSX-NEXT: xsmaddadp
 ; CHECK-SAFE-VSX-NEXT: xssubdp
 ; CHECK-SAFE-VSX-NEXT: blr
 
 ; CHECK-UNSAFE-VSX-LABEL: test_FMSUB_ASSOC2:
+; CHECK-UNSAFE-VSX: xsmuldp
+; CHECK-UNSAFE-VSX-NEXT: xsmaddadp
+; CHECK-UNSAFE-VSX-NEXT: xssubdp
+; CHECK-UNSAFE-VSX-NEXT: blr
+}
+
+define double @test_FMSUB_ASSOC2_NSZ(double %A, double %B, double %C,
+                                     double %D, double %E) {
+  %F = fmul nsz double %A, %B         ; <double> [#uses=1]
+  %G = fmul nsz double %C, %D         ; <double> [#uses=1]
+  %H = fadd nsz double %F, %G         ; <double> [#uses=1]
+  %I = fsub nsz double %E, %H         ; <double> [#uses=1]
+  ret double %I
+; CHECK-SAFE-LABEL: test_FMSUB_ASSOC2_NSZ:
+; CHECK-SAFE: fmul
+; CHECK-SAFE-NEXT: fmadd
+; CHECK-SAFE-NEXT: fsub
+; CHECK-SAFE-NEXT: blr
+
+; CHECK-UNSAFE-LABEL: test_FMSUB_ASSOC2_NSZ:
+; CHECK-UNSAFE: fnmsub
+; CHECK-UNSAFE-NEXT: fnmsub
+; CHECK-UNSAFE-NEXT: blr
+
+; CHECK-SAFE-VSX-LABEL: test_FMSUB_ASSOC2_NSZ:
+; CHECK-SAFE-VSX: xsmuldp
+; CHECK-SAFE-VSX-NEXT: xsmaddadp
+; CHECK-SAFE-VSX-NEXT: xssubdp
+; CHECK-SAFE-VSX-NEXT: blr
+
+; CHECK-UNSAFE-VSX-LABEL: test_FMSUB_ASSOC2_NSZ:
 ; CHECK-UNSAFE-VSX: xsnmsubmdp
 ; CHECK-UNSAFE-VSX-NEXT: xsnmsubadp
 ; CHECK-UNSAFE-VSX-NEXT: fmr
 ; CHECK-UNSAFE-VSX-NEXT: blr
 }
 
 define double @test_FMADD_ASSOC_EXT1(float %A, float %B, double %C,
                                  double %D, double %E) {