[DAG] Fold (umin (sub a b) a) -> (usubo a b); (select usubo.1 a usubo.0)

[ckoparkar]

Fixes #161036.

[ckoparkar]

/cc @RKSimon

[llvmbot]

@llvm/pr-subscribers-backend-aarch64
@llvm/pr-subscribers-llvm-selectiondag

@llvm/pr-subscribers-backend-x86

Author: Chaitanya Koparkar (ckoparkar)

Changes

Fixes #161036.

---

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

2 Files Affected:

• (modified) llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (+22)
• (added) llvm/test/CodeGen/X86/underflow-compare-fold.ll (+16)

diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index 558c5a0390228..99d7000c3b62e 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -6199,6 +6199,28 @@ SDValue DAGCombiner::visitIMINMAX(SDNode *N) {
                                         SDLoc(N), VT, N0, N1))
     return SD;
 
+  // (umin (sub a, b) a) -> (usubo a, b); (select usubo.1, a, usubo.0)
+  //
+  // IR:
+  //   %sub  = sub %a, %b
+  //   %cond = umin %sub, %a
+  //   ->
+  //   %usubo    = usubo %a, %b
+  //   %overflow = extractvalue %usubo, 1
+  //   %sub      = extractvalue %usubo, 0
+  //   %cond     = select %overflow, %a, %sub
+  if (N0.getOpcode() == ISD::SUB) {
+    SDValue A, B, C;
+    if (sd_match(N, m_UMin(m_Sub(m_Value(A), m_Value(B)), m_Value(C)))) {
+      EVT AVT = A.getValueType();
+      if (A == C && TLI.isOperationLegalOrCustom(ISD::USUBO, AVT)) {
+        SDVTList VTs = DAG.getVTList(AVT, MVT::i1);
+        SDValue USO = DAG.getNode(ISD::USUBO, DL, VTs, A, B);
+        return DAG.getSelect(DL, VT, USO.getValue(1), A, USO.getValue(0));
+      }
+    }
+  }
+
   // Simplify the operands using demanded-bits information.
   if (SimplifyDemandedBits(SDValue(N, 0)))
     return SDValue(N, 0);
diff --git a/llvm/test/CodeGen/X86/underflow-compare-fold.ll b/llvm/test/CodeGen/X86/underflow-compare-fold.ll
new file mode 100644
index 0000000000000..2416bcb909485
--- /dev/null
+++ b/llvm/test/CodeGen/X86/underflow-compare-fold.ll
@@ -0,0 +1,16 @@
+; RUN: llc < %s -mtriple=x86_64 | FileCheck %s
+
+; GitHub issue #161036
+
+define i64 @subIfNoUnderflow_umin(i64 %a, i64 %b) {
+; CHECK-LABEL: subIfNoUnderflow_umin
+; CHECK-LABEL: %bb.0
+; CHECK-NEXT: movq    %rdi, %rax
+; CHECK-NEXT: subq    %rsi, %rax
+; CHECK-NEXT: cmovbq  %rdi, %rax
+; CHECK-NEXT: retq
+entry:
+  %sub = sub i64 %a, %b
+  %cond = tail call i64 @llvm.umin.i64(i64 %sub, i64 %a)
+  ret i64 %cond
+}

[RKSimon]

please add aarch64 test coverage

[ckoparkar]

@RKSimon this is ready for another review.

[ckoparkar]

Apologies, I was traveling for a few days. I'll update this today.

[RKSimon]

Please can you update against trunk latest and investigate the CI failure

[ckoparkar]

1. Test failure

Previously the code was:

if (N0.getOpcode() == ISD::SUB) {
  sd_match(N, m_UMin(m_Sub(m_Value(A), m_Value(B)), m_Deferred(A)))
  ...

Once I removed the if (N0.getOpcode() == ISD::SUB), this pattern match grabbed this:

  SelectionDAG has 17 nodes:
  t2: i32,ch = CopyFromReg t0, Register:i32 %0
                t8: i32 = sub t2, Constant:i32<4>
              t9: i32 = umin t2, t8


N: t9: i32 = umin t2, t8
A: t2: i32,ch = CopyFromReg t0, Register:i32 %0
B: t7: i32 = Constant<4>

t8: i32 = sub.. is the second argument of umin so I'm kind of confused as to why this pattern match succeeded. I'm now pattern matching on individual arguments of umin.

2. More tests

I've added some tests with vector types, incorrect patterns, etc. @arsenm also mentioned a test for multiple use.

I didn't guard against multiple uses here since I thought the results of sub and umin are still available after the fold. Am I misunderstanding something here and should I be guarding the pattern with m_OneUse?

[ckoparkar]

To fix the test failure I need to update the patterns in CodeGen/AMDGPU/llvm.set.rounding.ll since the fold also applies to it. The DAG for set_rounding:

Legalized selection DAG: %bb.0 's_set_rounding:'
SelectionDAG has 17 nodes:
  t0: ch,glue = EntryToken
  t2: i32,ch = CopyFromReg t0, Register:i32 %0
                t8: i32 = sub t2, Constant:i32<4>
              t9: i32 = umin t2, t8
            t11: i32 = shl t9, Constant:i32<2>
          t12: i64 = srl Constant:i64<-5242644231586798321>, t11
        t13: i32 = truncate t12
      t15: i32 = llvm.amdgcn.readfirstlane TargetConstant:i32<3375>, t13
    t18: ch = llvm.amdgcn.s.setreg t0, TargetConstant:i32<3416>, TargetConstant:i32<6145>, t15
  t5: ch = RET_GLUE t18

[RKSimon]

LGTM

[mstorsjo]

This change broke a significant number of cases in ffmpeg, across multiple architectures.

One way of reproducing is this:

$ git clone https://github.com/ffmpeg/ffmpeg
$ mkdir ffmpeg-build
$ cd ffmpeg-build
$ ../ffmpeg/configure --cc=clang
$ make -j$(nproc)
$ make fate-vsynth1-flv

This produces a binary that crashes for this testcase.

On x86_64, the object file where the breakage is, is in libavcodec/x86/videodsp_init.o. On aarch64, the fault appears in libavcodec/videodsp.o.

Compilation of that particular object file (the one on x86) can be done standalone with https://martin.st/temp/videodsp_init-preproc.c - with clang -target x86_64-linux-gnu -c -O2 videodsp_init-preproc.c.

If comparing the difference in generated code caused by this patch, I get this:

--- out-good.s  2025-11-13 11:30:40.037589941 +0200
+++ out-bad.s   2025-11-13 11:30:33.665583286 +0200
@@ -132,15 +132,13 @@
        addq    %r10, %r11
        movq    %rax, %rbx
 .LBB1_12:                               # %if.end29.i
+       xorl    %eax, %eax
        subq    %r8, %r9
        negq    %r8
-       xorl    %eax, %eax
-       testq   %r8, %r8
-       cmovleq %rax, %r8
+       cmovbq  %rax, %r8
        movq    %rbx, %r13
        negq    %r13
-       testq   %r13, %r13
-       cmovleq %rax, %r13
+       cmovbq  %rax, %r13
        cmpq    %r9, %r14
        cmovlq  %r14, %r9
        subq    %rbx, %rbp
@@ -374,15 +372,13 @@
        addq    %r10, %r11
        movq    %rax, %rbx
 .LBB2_12:                               # %if.end29.i
+       xorl    %eax, %eax
        subq    %r8, %r9
        negq    %r8
-       xorl    %eax, %eax
-       testq   %r8, %r8
-       cmovleq %rax, %r8
+       cmovbq  %rax, %r8
        movq    %rbx, %r13
        negq    %r13
-       testq   %r13, %r13
-       cmovleq %rax, %r13
+       cmovbq  %rax, %r13
        cmpq    %r9, %r14
        cmovlq  %r14, %r9
        subq    %rbx, %rbp

I would suggest reverting this commit for now.