From fd9fa66239ebed0944ae94dde16f11f530347f17 Mon Sep 17 00:00:00 2001 From: Piotr Fusik Date: Fri, 16 May 2025 10:30:01 +0200 Subject: [PATCH 1/4] [RISCV][test] Add test for "subtraction if above a constant threshold" miscompile --- llvm/test/CodeGen/RISCV/rv32zbb.ll | 46 ++++++++++++++++++++++++++++++ llvm/test/CodeGen/RISCV/rv64zbb.ll | 46 ++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+) diff --git a/llvm/test/CodeGen/RISCV/rv32zbb.ll b/llvm/test/CodeGen/RISCV/rv32zbb.ll index 0f2284637ca6a..0586a851b4bff 100644 --- a/llvm/test/CodeGen/RISCV/rv32zbb.ll +++ b/llvm/test/CodeGen/RISCV/rv32zbb.ll @@ -1917,3 +1917,49 @@ 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: addi a0, a0, 17 +; 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: addi a0, a0, 17 +; 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 +} diff --git a/llvm/test/CodeGen/RISCV/rv64zbb.ll b/llvm/test/CodeGen/RISCV/rv64zbb.ll index 17eb0817d548a..bafd11d12e6b6 100644 --- a/llvm/test/CodeGen/RISCV/rv64zbb.ll +++ b/llvm/test/CodeGen/RISCV/rv64zbb.ll @@ -2072,3 +2072,49 @@ 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: addi a0, a0, 17 +; 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: addi a0, a0, 17 +; 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 +} From f0520642dddc76b48503a6774ada19c3908401fa Mon Sep 17 00:00:00 2001 From: Piotr Fusik Date: Thu, 15 May 2025 13:12:42 +0200 Subject: [PATCH 2/4] [DAGCombiner] Fix the "subtraction if above a constant threshold" miscompile This fixes #135194 incorrectly reusing the existing `add nuw/nsw` while the transformed code relies on an unsigned wrap. --- llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp | 15 ++++++++++----- llvm/test/CodeGen/RISCV/rv32zbb.ll | 6 ++++++ llvm/test/CodeGen/RISCV/rv64zbb.ll | 6 ++++++ 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp index d6e288a59b2ee..9d1a833575b53 100644 --- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp @@ -12140,11 +12140,16 @@ 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)) { - 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)))) + return DAG.getNode( + ISD::UMIN, DL, VT, + DAG.getNode(ISD::ADD, DL, VT, N2, DAG.getConstant(~C, DL, VT)), N2); + if (CC == ISD::SETULT && Cond0 == N1 && + sd_match(N2, m_Add(m_Specific(N1), m_SpecificInt(-C)))) + return DAG.getNode( + ISD::UMIN, DL, VT, N1, + DAG.getNode(ISD::ADD, DL, VT, N1, DAG.getConstant(-C, DL, VT))); } } diff --git a/llvm/test/CodeGen/RISCV/rv32zbb.ll b/llvm/test/CodeGen/RISCV/rv32zbb.ll index 0586a851b4bff..62bc7b3336a5c 100644 --- a/llvm/test/CodeGen/RISCV/rv32zbb.ll +++ b/llvm/test/CodeGen/RISCV/rv32zbb.ll @@ -1932,7 +1932,10 @@ define i7 @sub_if_uge_C_nsw_i7(i7 %a) { ; 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 @@ -1955,7 +1958,10 @@ define i7 @sub_if_uge_C_swapped_nsw_i7(i7 %a) { ; 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 diff --git a/llvm/test/CodeGen/RISCV/rv64zbb.ll b/llvm/test/CodeGen/RISCV/rv64zbb.ll index bafd11d12e6b6..a04d99d57cec1 100644 --- a/llvm/test/CodeGen/RISCV/rv64zbb.ll +++ b/llvm/test/CodeGen/RISCV/rv64zbb.ll @@ -2087,7 +2087,10 @@ define i7 @sub_if_uge_C_nsw_i7(i7 %a) { ; 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 @@ -2110,7 +2113,10 @@ define i7 @sub_if_uge_C_swapped_nsw_i7(i7 %a) { ; 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 From 74ca22ce69f9bec0685c8f1f4f52229f142e6d13 Mon Sep 17 00:00:00 2001 From: Piotr Fusik Date: Fri, 16 May 2025 11:18:03 +0200 Subject: [PATCH 3/4] [DAGCombiner] Prettify with braces and temporaries --- llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp index 9d1a833575b53..675a54f2a3c06 100644 --- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp @@ -12141,15 +12141,17 @@ SDValue DAGCombiner::visitSELECT(SDNode *N) { APInt C; if (sd_match(Cond1, m_ConstInt(C)) && hasUMin(VT)) { if (CC == ISD::SETUGT && Cond0 == N2 && - sd_match(N1, m_Add(m_Specific(N2), m_SpecificInt(~C)))) - return DAG.getNode( - ISD::UMIN, DL, VT, - DAG.getNode(ISD::ADD, DL, VT, N2, DAG.getConstant(~C, DL, VT)), N2); + sd_match(N1, m_Add(m_Specific(N2), m_SpecificInt(~C)))) { + 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 && - sd_match(N2, m_Add(m_Specific(N1), m_SpecificInt(-C)))) - return DAG.getNode( - ISD::UMIN, DL, VT, N1, - DAG.getNode(ISD::ADD, DL, VT, N1, DAG.getConstant(-C, DL, VT))); + sd_match(N2, m_Add(m_Specific(N1), m_SpecificInt(-C)))) { + 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); + } } } From b61f74e670075233dbc24e6f47bd6495ba528191 Mon Sep 17 00:00:00 2001 From: Piotr Fusik Date: Fri, 16 May 2025 11:45:00 +0200 Subject: [PATCH 4/4] [DAGCombiner] Explain why we recreate ADD --- llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp index 675a54f2a3c06..1d4a3e58e567c 100644 --- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp @@ -12142,12 +12142,15 @@ SDValue DAGCombiner::visitSELECT(SDNode *N) { if (sd_match(Cond1, m_ConstInt(C)) && hasUMin(VT)) { if (CC == ISD::SETUGT && Cond0 == N2 && sd_match(N1, m_Add(m_Specific(N2), m_SpecificInt(~C)))) { + // 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 && 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);