[DAGCombiner] Fix abs(add) to abdu miscompile in foldABSToABD

[el-ev]

The abs(add(x, y)) → abdu(x, -y) fold added in #186659 is incorrect when both operands are known non-negative and their sum does not overflow signed.

When both x and y are non-negative and x + y < 2^31, abs(x + y) = x + y, but abdu(x, -y) = 2^32 - y - x ≠ x + y.

For example, abs(add(0, 1)) = 1, but abdu(0, -1) = 0xFFFFFFFF.

cc @DaKnig

Related: #185467 #175801

Found while working on #196786

[el-ev]

• [SelectionDAG] Emit AssertZext for function argument range attributes #196786
• [SelectionDAG] Drop unnecessary lower bound check in lowerRangeToAssertZExt #196785
• [DAGCombiner] Fix abs(add) to abdu miscompile in foldABSToABD #196782 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[llvmorg-github-actions]

@llvm/pr-subscribers-llvm-selectiondag

@llvm/pr-subscribers-backend-x86

Author: Iris Shi (el-ev)

Changes

The abs(add(x, y)) → abdu(x, -y) fold added in #186659 is incorrect when both operands are known non-negative and their sum does not overflow signed.

When both x and y are non-negative and x + y < 2^31, abs(x + y) = x + y, but abdu(x, -y) = 2^32 - y - x ≠ x + y.

For example, abs(add(0, 1)) = 1, but abdu(0, -1) = 0xFFFFFFFF.

cc @DaKnig

---

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

3 Files Affected:

• (modified) llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (+1-3)
• (modified) llvm/test/CodeGen/X86/abds.ll (+26)
• (added) shell.nix (+34)

diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index 5a467a5a5ba53..5dd025f1b42be 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -12025,10 +12025,8 @@ SDValue DAGCombiner::foldABSToABD(SDNode *N, const SDLoc &DL) {
       return CreateZextedAbd(ISD::ABDS);
 
     // fold (abs (sub x, y)) -> abdu(x, y)
-    // fold (abs (add x, -y)) -> abdu(x, y)
     bool Op1SignBitIsOne = DAG.computeKnownBits(Op1).isNegative();
-    bool AbsOpWillNUW = DAG.SignBitIsZero(Op0) &&
-                        (IsAdd ? DAG.SignBitIsZero(Op1) : Op1SignBitIsOne);
+    bool AbsOpWillNUW = !IsAdd && DAG.SignBitIsZero(Op0) && Op1SignBitIsOne;
 
     if (hasOperation(ISD::ABDU, VT) && AbsOpWillNUW)
       return CreateZextedAbd(ISD::ABDU);
diff --git a/llvm/test/CodeGen/X86/abds.ll b/llvm/test/CodeGen/X86/abds.ll
index 5948af563d152..a3056e9426643 100644
--- a/llvm/test/CodeGen/X86/abds.ll
+++ b/llvm/test/CodeGen/X86/abds.ll
@@ -1389,6 +1389,32 @@ define i32 @PR185467(i32 range(i32 0, 2147483647) %v) {
   ret i32 %absx
 }
 
+define i32 @abs_add_known_positive(i32 %a) {
+; X86-LABEL: abs_add_known_positive:
+; X86:       # %bb.0:
+; X86-NEXT:    movl $2147483647, %ecx # imm = 0x7FFFFFFF
+; X86-NEXT:    andl {{[0-9]+}}(%esp), %ecx
+; X86-NEXT:    incl %ecx
+; X86-NEXT:    movl %ecx, %eax
+; X86-NEXT:    negl %eax
+; X86-NEXT:    cmovsl %ecx, %eax
+; X86-NEXT:    retl
+;
+; X64-LABEL: abs_add_known_positive:
+; X64:       # %bb.0:
+; X64-NEXT:    # kill: def $edi killed $edi def $rdi
+; X64-NEXT:    andl $2147483647, %edi # imm = 0x7FFFFFFF
+; X64-NEXT:    leal 1(%rdi), %ecx
+; X64-NEXT:    movl %ecx, %eax
+; X64-NEXT:    negl %eax
+; X64-NEXT:    cmovsl %ecx, %eax
+; X64-NEXT:    retq
+  %x = and i32 %a, 2147483647
+  %add = add i32 %x, 1
+  %abs = call i32 @llvm.abs.i32(i32 %add, i1 false)
+  ret i32 %abs
+}
+
 declare i8 @llvm.abs.i8(i8, i1)
 declare i16 @llvm.abs.i16(i16, i1)
 declare i32 @llvm.abs.i32(i32, i1)
diff --git a/shell.nix b/shell.nix
new file mode 100644
index 0000000000000..7615218cfce56
--- /dev/null
+++ b/shell.nix
@@ -0,0 +1,34 @@
+with import <nixpkgs> {};
+let
+  gccForLibs = stdenv.cc.cc;
+in stdenv.mkDerivation {
+  name = "llvm-env";
+  buildInputs = [
+    python3
+    ninja
+    cmake
+    ccache
+  ];
+
+  shellHook = ''
+    configure() {
+      eval "cmake $cmakeFlags ../llvm $*"
+    }
+  '';
+
+  # where to find libgcc
+  NIX_LDFLAGS="-L${gccForLibs}/lib/gcc/${targetPlatform.config}/${gccForLibs.version}";
+  # teach clang about C startup file locations
+  CFLAGS="-B${gccForLibs}/lib/gcc/${targetPlatform.config}/${gccForLibs.version} -B ${stdenv.cc.libc}/lib";
+
+  cmakeFlags = [
+    "-DC_INCLUDE_DIRS=${stdenv.cc.libc.dev}/include"
+    "-GNinja"
+    "-DCMAKE_BUILD_TYPE=Release"
+    "-DCMAKE_INSTALL_PREFIX=../inst"
+    "-DLLVM_ENABLE_PROJECTS=\"clang;llvm\""
+    "-DLLVM_TARGETS_TO_BUILD=host"
+    "-DCMAKE_C_COMPILER_LAUNCHER=ccache"
+    "-DCMAKE_CXX_COMPILER_LAUNCHER=ccache"
+  ];
+}

[github-actions]

🪟 Windows x64 Test Results

• 134519 tests passed
• 3259 tests skipped

✅ The build succeeded and all tests passed.

[github-actions]

🐧 Linux x64 Test Results

• 195126 tests passed
• 5196 tests skipped

✅ The build succeeded and all tests passed.

[RKSimon]

LGTM

[el-ev]

It would be good to add the fold back later with a proper check that x + y actually overflows.