[llvm] [DAGCombiner] Fix the "subtraction if above a constant threshold" miscompile (PR #140042)

Piotr Fusik via llvm-commits llvm-commits at lists.llvm.org
Fri May 16 02:45:54 PDT 2025


https://github.com/pfusik updated https://github.com/llvm/llvm-project/pull/140042

>From fd9fa66239ebed0944ae94dde16f11f530347f17 Mon Sep 17 00:00:00 2001
From: Piotr Fusik <p.fusik at samsung.com>
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 <p.fusik at samsung.com>
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 <p.fusik at samsung.com>
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 <p.fusik at samsung.com>
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);



More information about the llvm-commits mailing list