[RISCV] Propagate SDNode flags when combining (fmul (fneg X), ...)

[mshockwave]

In #157388, we turned (fmul (fneg X), Y) into (fneg (fmul X, Y)). However, we forgot to propagate SDNode flags, specifically fast math flags, from the original FMUL to the new one. This hinders some of the subsequent (FMA) DAG combiner patterns that relied on the contraction flag and as a consequence, missed some of the opportunities to generate negation FMA instructions like fnmadd.

This patch fixes this issue by propagating the flags.

[llvmbot]

@llvm/pr-subscribers-backend-risc-v

Author: Min-Yih Hsu (mshockwave)

Changes

In #157388, we turned (fmul (fneg X), Y) into (fneg (fmul X, Y)). However, we forgot to propagate SDNode flags, specifically fast math flags, from the original FMUL to the new one. This hinders some of the subsequent (FMA) DAG combiner patterns that relied on the contraction flag and as a consequence, missed some of the opportunities to generate negation FMA instructions like fnmadd.

This patch fixes this issue by propagating the flags.

---

Full diff: https://github.com/llvm/llvm-project/pull/169460.diff

2 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+1-1)
• (added) llvm/test/CodeGen/RISCV/fma-combine.ll (+52)

diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index 19a16197272fe..53293fb093ccd 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -20822,7 +20822,7 @@ SDValue RISCVTargetLowering::PerformDAGCombine(SDNode *N,
     // Undo this and sink the fneg so we match more fmsub/fnmadd patterns.
     if (sd_match(N, m_FMul(m_Value(X), m_OneUse(m_FNeg(m_Value(Y))))))
       return DAG.getNode(ISD::FNEG, DL, VT,
-                         DAG.getNode(ISD::FMUL, DL, VT, X, Y));
+                         DAG.getNode(ISD::FMUL, DL, VT, X, Y, N->getFlags()));
 
     // fmul X, (copysign 1.0, Y) -> fsgnjx X, Y
     SDValue N0 = N->getOperand(0);
diff --git a/llvm/test/CodeGen/RISCV/fma-combine.ll b/llvm/test/CodeGen/RISCV/fma-combine.ll
new file mode 100644
index 0000000000000..67d3175718072
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/fma-combine.ll
@@ -0,0 +1,52 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 6
+; RUN: llc -mtriple=riscv64 -mattr='+d,+m' < %s | FileCheck %s
+
+define double @fnmadd_non_trivial(ptr %p0, ptr %p1, ptr %dst, double %mul425) {
+; CHECK-LABEL: fnmadd_non_trivial:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    li a3, -2047
+; CHECK-NEXT:    slli a3, a3, 51
+; CHECK-NEXT:    fmv.d.x fa5, a3
+; CHECK-NEXT:    lui a3, 2049
+; CHECK-NEXT:    slli a3, a3, 39
+; CHECK-NEXT:    fmv.d.x fa4, a3
+; CHECK-NEXT:    lui a3, 8201
+; CHECK-NEXT:    slli a3, a3, 37
+; CHECK-NEXT:    fmv.d.x fa3, a3
+; CHECK-NEXT:    li a3, 1023
+; CHECK-NEXT:    fmv.d.x fa2, zero
+; CHECK-NEXT:    slli a3, a3, 52
+; CHECK-NEXT:    fsub.d fa1, fa2, fa0
+; CHECK-NEXT:    fmadd.d fa1, fa1, fa3, fa4
+; CHECK-NEXT:    fmadd.d fa4, fa0, fa3, fa4
+; CHECK-NEXT:    fmv.d.x fa3, a3
+; CHECK-NEXT:    lui a3, %hi(.LCPI0_0)
+; CHECK-NEXT:    ld a3, %lo(.LCPI0_0)(a3)
+; CHECK-NEXT:    fmul.d fa5, fa0, fa5
+; CHECK-NEXT:    fnmadd.d fa4, fa4, fa2, fa3
+; CHECK-NEXT:    fnmadd.d fa3, fa1, fa2, fa3
+; CHECK-NEXT:    sd a3, 0(a2)
+; CHECK-NEXT:    fsd fa5, 0(a0)
+; CHECK-NEXT:    fnmadd.d fa5, fa4, fa2, fa0
+; CHECK-NEXT:    fnmadd.d fa0, fa0, fa2, fa3
+; CHECK-NEXT:    fsd fa5, 0(a1)
+; CHECK-NEXT:    ret
+  store double 0x3FEE666666666666, ptr %dst, align 8
+  %mul413 = fmul double %mul425, -3.000000e+00
+  store double %mul413, ptr %p0, align 8
+  %mul428 = fmul contract double %mul425, 4.500000e+00
+  %add429 = fadd nsz contract double %mul428, 3.000000e+00
+  %mul430 = fmul contract double %add429, 0.000000e+00
+  %sub432 = fadd nsz contract double %mul430, 1.000000e+00
+  %mul433 = fmul contract double %sub432, 0.000000e+00
+  %1 = fsub nsz contract double %mul433, %mul425
+  store double %1, ptr %p1, align 8
+  %mul441 = fmul contract double %mul425, 0.000000e+00
+  %add443 = fsub double 0.000000e+00, %mul425
+  %mul446 = fmul contract double %add443, 4.500000e+00
+  %add447 = fadd nsz contract double %mul446, 3.000000e+00
+  %mul448 = fmul contract double %add447, 0.000000e+00
+  %sub450 = fadd nsz contract double %mul448, 1.000000e+00
+  %2 = fsub nsz contract double %sub450, %mul441
+  ret double %2
+}

[mshockwave]

context: the issue was originally found in 470.lbm, from which this test was reduced.
The reason this test looks so big and weird in some sense was because the flaw (i.e. missing SDNode flags) can only be revealed when a specific set of FMA DAG combiner patterns were skipped due to hitting some recursion limits. And this test is written in a way to hit that limit. I have spend lots of time trying other ways to test this but to no avail. An alternative could be directly checking the fast math flags in the SelectionDAG dump but I don't think that's a better idea.

[topperc]

Probably worth a comment here.

[mshockwave]

Done

[lukel97]

LGTM

[lukel97]

Nit are the quotes needed?

Suggested change

	; RUN: llc -mtriple=riscv64 -mattr='+d,+m' < %s | FileCheck %s
	; RUN: llc -mtriple=riscv64 -mattr=+d,+m < %s | FileCheck %s

[mshockwave]

oops, they're removed now.