Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 25, 2025
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
95836ed
Pre-commit test
mshockwave Nov 25, 2025
6992293
[RISCV] Propagate SDNode flags when combining `(fmul (fneg ...))`
mshockwave Nov 25, 2025
98be560
fixup! Address review comments
mshockwave Nov 25, 2025
a0649a9
fixup! Remove quotes in the test
mshockwave Nov 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion llvm/lib/Target/RISCV/RISCVISelLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20822,7 +20822,8 @@ 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()),
N->getFlags());

// fmul X, (copysign 1.0, Y) -> fsgnjx X, Y
SDValue N0 = N->getOperand(0);
Expand Down
56 changes: 56 additions & 0 deletions llvm/test/CodeGen/RISCV/fma-combine.ll
View file
Open in desktop
Copy link
Member Author

@mshockwave mshockwave Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
; 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
Copy link
Contributor

@lukel97 lukel97 Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@mshockwave mshockwave Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, they're removed now.


; What the original PR (#169460) tried to solve 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.

define double @fnmadd_non_trivial(ptr %p0, ptr %p1, ptr %dst, double %mul425) {
Copy link
Collaborator

@topperc topperc Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth a comment here.

Copy link
Member Author

@mshockwave mshockwave Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

; 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
}
Loading