Skip to content

[DAGCombiner] Fix the "subtraction if above a constant threshold" miscompile #140042

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
May 17, 2025
Merged

[DAGCombiner] Fix the "subtraction if above a constant threshold" miscompile #140042

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fd9fa66
[RISCV][test] Add test for "subtraction if above a constant threshold…
pfusik May 16, 2025
f052064
[DAGCombiner] Fix the "subtraction if above a constant threshold" mis…
pfusik May 15, 2025
74ca22c
[DAGCombiner] Prettify with braces and temporaries
pfusik May 16, 2025
b61f74e
[DAGCombiner] Explain why we recreate ADD
pfusik May 16, 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
20 changes: 15 additions & 5 deletions llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12140,11 +12140,21 @@ SDValue DAGCombiner::visitSELECT(SDNode *N) {
// (select (ult x, C), x, (add x, -C)) -> (umin x, (add x, -C))
APInt C;
if (sd_match(Cond1, m_ConstInt(C)) && hasUMin(VT)) {
Copy link
Collaborator

@bjope bjope May 16, 2025

Choose a reason for hiding this comment

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

nit: Could add a small comment here telling why we need to recreate the ADD. Just to avoid that someone tries to simplify this in the future, or if someone wonder why we can do this without introducing FREEZE.

Maybe something like this:
// Need to drop any poison generating flags from the ADD, to ensure that the result isn't more poisonous.

Copy link
Contributor Author

@pfusik pfusik May 16, 2025

Choose a reason for hiding this comment

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

Done.

if ((CC == ISD::SETUGT && Cond0 == N2 &&
sd_match(N1, m_Add(m_Specific(N2), m_SpecificInt(~C)))) ||
(CC == ISD::SETULT && Cond0 == N1 &&
sd_match(N2, m_Add(m_Specific(N1), m_SpecificInt(-C)))))
return DAG.getNode(ISD::UMIN, DL, VT, N1, N2);
if (CC == ISD::SETUGT && Cond0 == N2 &&
sd_match(N1, m_Add(m_Specific(N2), m_SpecificInt(~C)))) {
Copy link
Contributor

@arsenm arsenm May 16, 2025

Choose a reason for hiding this comment

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

Braces, and/or use some temporary values to reduce this ugly wrapping

Copy link
Contributor Author

@pfusik pfusik May 16, 2025

Choose a reason for hiding this comment

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

Done.

// The resulting code relies on an unsigned wrap in ADD.
// Recreating ADD to drop possible nuw/nsw flags.
SDValue AddC = DAG.getConstant(~C, DL, VT);
SDValue Add = DAG.getNode(ISD::ADD, DL, VT, N2, AddC);
return DAG.getNode(ISD::UMIN, DL, VT, Add, N2);
}
if (CC == ISD::SETULT && Cond0 == N1 &&
Copy link
Collaborator

@RKSimon RKSimon May 15, 2025

Choose a reason for hiding this comment

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

It might be cleaner just to split these and then do:

if (CC == ISD::SETULT && Cond0 == N1 &&
    sd_match(N2, m_Add(m_Specific(N1), m_SpecificInt(-C))))
 return DAG.getNode(ISD::UMIN, DL, VT, Cond0, DAG.getNode(ISD::ADD, DL, VT, Cond0,
                                    DAG.getConstant(-C, DL, VT)));

etc.

Copy link
Contributor Author

@pfusik pfusik May 15, 2025

Choose a reason for hiding this comment

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

Done.

sd_match(N2, m_Add(m_Specific(N1), m_SpecificInt(-C)))) {
// Ditto.
SDValue AddC = DAG.getConstant(-C, DL, VT);
SDValue Add = DAG.getNode(ISD::ADD, DL, VT, N1, AddC);
return DAG.getNode(ISD::UMIN, DL, VT, N1, Add);
}
}
}

Expand Down
52 changes: 52 additions & 0 deletions llvm/test/CodeGen/RISCV/rv32zbb.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1917,3 +1917,55 @@ define i32 @sub_if_uge_C_swapped_i32(i32 %x) {
%cond = select i1 %cmp, i32 %x, i32 %sub
ret i32 %cond
}

define i7 @sub_if_uge_C_nsw_i7(i7 %a) {
; RV32I-LABEL: sub_if_uge_C_nsw_i7:
; RV32I: # %bb.0:
; RV32I-NEXT: ori a0, a0, 51
; RV32I-NEXT: andi a1, a0, 127
; RV32I-NEXT: sltiu a1, a1, 111
; RV32I-NEXT: addi a1, a1, -1
; RV32I-NEXT: andi a1, a1, 17
; RV32I-NEXT: add a0, a0, a1
; RV32I-NEXT: ret
;
; RV32ZBB-LABEL: sub_if_uge_C_nsw_i7:
; RV32ZBB: # %bb.0:
; RV32ZBB-NEXT: ori a0, a0, 51
; RV32ZBB-NEXT: andi a1, a0, 127
; RV32ZBB-NEXT: addi a0, a0, 17
; RV32ZBB-NEXT: andi a0, a0, 92
; RV32ZBB-NEXT: minu a0, a0, a1
; RV32ZBB-NEXT: ret
%x = or i7 %a, 51
%c = icmp ugt i7 %x, -18
%add = add nsw i7 %x, 17
%s = select i1 %c, i7 %add, i7 %x
ret i7 %s
}

define i7 @sub_if_uge_C_swapped_nsw_i7(i7 %a) {
; RV32I-LABEL: sub_if_uge_C_swapped_nsw_i7:
; RV32I: # %bb.0:
; RV32I-NEXT: ori a0, a0, 51
; RV32I-NEXT: andi a1, a0, 127
; RV32I-NEXT: sltiu a1, a1, 111
; RV32I-NEXT: addi a1, a1, -1
; RV32I-NEXT: andi a1, a1, 17
; RV32I-NEXT: add a0, a0, a1
; RV32I-NEXT: ret
;
; RV32ZBB-LABEL: sub_if_uge_C_swapped_nsw_i7:
; RV32ZBB: # %bb.0:
; RV32ZBB-NEXT: ori a0, a0, 51
; RV32ZBB-NEXT: andi a1, a0, 127
; RV32ZBB-NEXT: addi a0, a0, 17
; RV32ZBB-NEXT: andi a0, a0, 92
; RV32ZBB-NEXT: minu a0, a1, a0
; RV32ZBB-NEXT: ret
%x = or i7 %a, 51
%c = icmp ult i7 %x, -17
%add = add nsw i7 %x, 17
%s = select i1 %c, i7 %x, i7 %add
ret i7 %s
}
52 changes: 52 additions & 0 deletions llvm/test/CodeGen/RISCV/rv64zbb.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2072,3 +2072,55 @@ define i32 @sub_if_uge_C_swapped_i32(i32 signext %x) {
%cond = select i1 %cmp, i32 %x, i32 %sub
ret i32 %cond
}

define i7 @sub_if_uge_C_nsw_i7(i7 %a) {
; RV64I-LABEL: sub_if_uge_C_nsw_i7:
; RV64I: # %bb.0:
; RV64I-NEXT: ori a0, a0, 51
; RV64I-NEXT: andi a1, a0, 127
; RV64I-NEXT: sltiu a1, a1, 111
; RV64I-NEXT: addi a1, a1, -1
; RV64I-NEXT: andi a1, a1, 17
; RV64I-NEXT: add a0, a0, a1
; RV64I-NEXT: ret
;
; RV64ZBB-LABEL: sub_if_uge_C_nsw_i7:
; RV64ZBB: # %bb.0:
; RV64ZBB-NEXT: ori a0, a0, 51
; RV64ZBB-NEXT: andi a1, a0, 127
; RV64ZBB-NEXT: addi a0, a0, 17
; RV64ZBB-NEXT: andi a0, a0, 92
; RV64ZBB-NEXT: minu a0, a0, a1
; RV64ZBB-NEXT: ret
%x = or i7 %a, 51
%c = icmp ugt i7 %x, -18
%add = add nsw i7 %x, 17
%s = select i1 %c, i7 %add, i7 %x
ret i7 %s
}

define i7 @sub_if_uge_C_swapped_nsw_i7(i7 %a) {
; RV64I-LABEL: sub_if_uge_C_swapped_nsw_i7:
; RV64I: # %bb.0:
; RV64I-NEXT: ori a0, a0, 51
; RV64I-NEXT: andi a1, a0, 127
; RV64I-NEXT: sltiu a1, a1, 111
; RV64I-NEXT: addi a1, a1, -1
; RV64I-NEXT: andi a1, a1, 17
; RV64I-NEXT: add a0, a0, a1
; RV64I-NEXT: ret
;
; RV64ZBB-LABEL: sub_if_uge_C_swapped_nsw_i7:
; RV64ZBB: # %bb.0:
; RV64ZBB-NEXT: ori a0, a0, 51
; RV64ZBB-NEXT: andi a1, a0, 127
; RV64ZBB-NEXT: addi a0, a0, 17
; RV64ZBB-NEXT: andi a0, a0, 92
; RV64ZBB-NEXT: minu a0, a1, a0
; RV64ZBB-NEXT: ret
%x = or i7 %a, 51
%c = icmp ult i7 %x, -17
%add = add nsw i7 %x, 17
%s = select i1 %c, i7 %x, i7 %add
ret i7 %s
}