[DAGCombiner] Check term use before applying aggressive FSUB optimisation

[critson]

Summary

Without this check unnecessary FMA instructions are generated when the FSUB RHS term is reused.
This also has the side-effect that the same value is computed to different levels of precision, which can create undesirable effects if the results are used together in subsequent computation.

[foad]

Looks like this case was missed in rL291642. Is it worth checking the other cases too, e.g. the next one at line 11896? Can you add your test to fadd-fma-fmul-combine.ll instead of creating a new test file?

[critson]

In D71656#1790846, @foad wrote:

Looks like this case was missed in rL291642. Is it worth checking the other cases too, e.g. the next one at line 11896? Can you add your test to fadd-fma-fmul-combine.ll instead of creating a new test file?

Thanks for pointing this out. I'll review the other cases and integrate the tests into the same file. It looks like the test I created is just a mirror of the previous test with LHS and RHS swapped.

[critson]

Unify with existing tests and apply usage check to further similar patterns.

[arsenm]

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

258 ↗

(On Diff #234826)

s/CHECK/GCN

[arsenm]

LGTM with check prefix fix

[foad]

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

258 ↗

(On Diff #234826)

s/GCN/GCN-LABEL/ :-)

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (CanFuse && N0.getOpcode() == PreferredFusedOpcode &&
                                      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))) {
+        isContractableFMUL(N1.getOperand(2)) &&
+        N1->hasOneUse()) {
       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/dagcombine-excessive-fma.ll

+;RUN: llc < %s -march=amdgcn -mcpu=gfx1010 | FileCheck %s
+
+declare float @llvm.sqrt.f32(float) #0
+
+; CHECK: {{^}}excess_fma:
+; CHECK: v_sub_f32_e32
+; CHECK: v_sub_f32_e32
+define amdgpu_cs float @excess_fma(float inreg %0, float inreg %1, float inreg %2, float inreg %3) local_unnamed_addr {
+  %.i185 = fmul reassoc nnan nsz arcp contract float %1, 0x3FF5F91700000000
+  %.i286 = fmul reassoc nnan nsz arcp contract float %2, 0x3FB9264240000000
+  %.v0 = fadd reassoc nnan nsz arcp contract float %3, %.i185
+  %.v1 = fadd reassoc nnan nsz arcp contract float %.i286, %.v0
+  %.i087 = fmul reassoc nnan nsz arcp contract float %0, 0x3E65798EE0000000
+  %.i188 = fmul reassoc nnan nsz arcp contract float %2, 0x3FEFB85660000000
+  %.v2 = fadd reassoc nnan nsz arcp contract float %.i087, %.i188
+  %.i0101 = fsub reassoc nnan nsz arcp contract float %.v2, %.v1
+  %.i1102 = fsub reassoc nnan nsz arcp contract float %.v1, %3
+  %.i2103 = fsub reassoc nnan nsz arcp contract float %3, %.v2
+  %.i0106 = fmul reassoc nnan nsz arcp contract float %.i0101, %.v2
+  %.i1107 = fmul reassoc nnan nsz arcp contract float %.i1102, %.v1
+  %.v3 = fadd reassoc nnan nsz arcp contract float %.i1107, %.i0106
+  %.v4 = fmul reassoc nnan nsz arcp contract float %3, %.i2103
+  %.v5 = fadd reassoc nnan nsz arcp contract float %.v4, %.v3
+  %.root = call reassoc nnan nsz arcp contract float @llvm.sqrt.f32(float %.v5)
+  ret float %.root
+}
+
+attributes #0 = { nounwind readnone speculatable willreturn }