[SDAG] Fix invalid sign bit condition for abs(sub) -> abdu fold - #215548
Conversation
The fold here for (abs (sub x y)) -> (abdu x y) was proven in Alive, assuming that both operands had a sign bit of zero. However, the code was checking if x had a sign bit of zero and y had a sign bit of 1 Fixes llvm#214942
llvmorg-github-actions
Bot
added
backend:X86
llvm:SelectionDAG
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-llvm-selectiondag Author: Benji Smith (Benjins) ChangesThe fold here for (abs (sub x y)) -> (abdu x y) was proven in Alive, assuming that both operands had a sign bit of zero. However, the code was checking if x had a sign bit of zero and y had a sign bit of 1 Fixes #214942 Original Alive proof from #186659 : https://alive2.llvm.org/ce/z/HfPF5q See also previously: #196782 which seemed to fix a variant of this in the abs(add) path Also, I assume this will want to be cherry-picked for the 23.x release branch since it's a regression from 22.x cc @DaKnig Full diff: https://github.com/llvm/llvm-project/pull/215548.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index 1095464a1ebdd..70645a5bf1714 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -12237,8 +12237,8 @@ SDValue DAGCombiner::foldABSToABD(SDNode *N, const SDLoc &DL) {
return CreateZextedAbd(ISD::ABDS);
// fold (abs (sub x, y)) -> abdu(x, y)
- bool Op1SignBitIsOne = DAG.computeKnownBits(Op1).isNegative();
- bool AbsOpWillNUW = !IsAdd && DAG.SignBitIsZero(Op0) && Op1SignBitIsOne;
+ bool AbsOpWillNUW =
+ !IsAdd && DAG.SignBitIsZero(Op0) && DAG.SignBitIsZero(Op1);
if (hasOperation(ISD::ABDU, VT) && AbsOpWillNUW)
return CreateZextedAbd(ISD::ABDU);
diff --git a/llvm/test/CodeGen/X86/abdu.ll b/llvm/test/CodeGen/X86/abdu.ll
index b8bc3649773f2..520a6917aff53 100644
--- a/llvm/test/CodeGen/X86/abdu.ll
+++ b/llvm/test/CodeGen/X86/abdu.ll
@@ -949,6 +949,36 @@ define i128 @abd_select_i128(i128 %a, i128 %b) nounwind {
ret i128 %sub
}
+define i32 @abs_sub_abdu_sign_check(i32 %p0) {
+; X86-LABEL: abs_sub_abdu_sign_check:
+; X86: # %bb.0: # %entry
+; X86-NEXT: cmpl $0, {{[0-9]+}}(%esp)
+; X86-NEXT: movl $1000000000, %eax # imm = 0x3B9ACA00
+; X86-NEXT: movl $-2039640824, %ecx # imm = 0x866D8D08
+; X86-NEXT: cmovel %eax, %ecx
+; X86-NEXT: movl %ecx, %eax
+; X86-NEXT: negl %eax
+; X86-NEXT: cmovsl %ecx, %eax
+; X86-NEXT: retl
+;
+; X64-LABEL: abs_sub_abdu_sign_check:
+; X64: # %bb.0: # %entry
+; X64-NEXT: testl %edi, %edi
+; X64-NEXT: movl $1000000000, %eax # imm = 0x3B9ACA00
+; X64-NEXT: movl $-2039640824, %ecx # imm = 0x866D8D08
+; X64-NEXT: cmovel %eax, %ecx
+; X64-NEXT: movl %ecx, %eax
+; X64-NEXT: negl %eax
+; X64-NEXT: cmovsl %ecx, %eax
+; X64-NEXT: retq
+entry:
+ %cmp = icmp eq i32 %p0, 0
+ %v = select i1 %cmp, i32 0, i32 1255326472
+ %s = sub i32 %v, -1000000000
+ %a = call i32 @llvm.abs.i32(i32 %s, i1 false)
+ ret i32 %a
+}
+
declare i8 @llvm.abs.i8(i8, i1)
declare i16 @llvm.abs.i16(i16, i1)
declare i32 @llvm.abs.i32(i32, i1)
|
|
When #186659 added Prior to that there was a |
|
That sounds to be right.
…On August 13, 2026 10:24:09 PM GMT+02:00, Craig Topper ***@***.***> wrote:
topperc left a comment (llvm/llvm-project#215548)
When #186659 added `fold (abs (add x, -y)) -> abdu(x, y)` did it get this conditional operator reversed?
````
bool Op1SignBitIsOne = DAG.computeKnownBits(Op1).isNegative();
bool AbsOpWillNUW = DAG.SignBitIsZero(Op0) &&
(IsAdd ? DAG.SignBitIsZero(Op1) : Op1SignBitIsOne);
```
Prior to that there was a `DAG.SignBitIsZero(Op1)`, but after it looks like the Op1SignBitIsOne is applied to sub?
--
Reply to this email directly or view it on GitHub:
#215548 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
|
Also on that note, now that you bring it up, the name of the variable is poor, it isnt "AbsOpWillNUW" but more like "AbsOpInSignedRange": if we invert the conditional (as @Topper suggested), the test checks that the mathematical result of a±b is within [INT_MAX, -INT_MAX] (INT_MIN is ruled out for the add case, separately, above.). The current name "... WillNUW" implies that we care if AbsOp goes below zero, but it doesn't matter at all.
That is what we need to test, to apply ABDU correctly here (inverting Op1 when needed).
…On August 14, 2026 9:57:33 AM GMT+02:00, natanelho ***@***.***> wrote:
That sounds to be right.
On August 13, 2026 10:24:09 PM GMT+02:00, Craig Topper ***@***.***> wrote:
>topperc left a comment (llvm/llvm-project#215548)
>
>When #186659 added `fold (abs (add x, -y)) -> abdu(x, y)` did it get this conditional operator reversed?
>
>````
> bool Op1SignBitIsOne = DAG.computeKnownBits(Op1).isNegative();
> bool AbsOpWillNUW = DAG.SignBitIsZero(Op0) &&
> (IsAdd ? DAG.SignBitIsZero(Op1) : Op1SignBitIsOne);
>```
>
>Prior to that there was a `DAG.SignBitIsZero(Op1)`, but after it looks like the Op1SignBitIsOne is applied to sub?
>
>--
>Reply to this email directly or view it on GitHub:
>#215548 (comment)
>You are receiving this because you were mentioned.
>
>Message ID: ***@***.***>
|
|
The patch, as is, is incorrent for the case of IsAdd.
Please change the code as described by @topperc :
```
bool Op1SignBitIsOne = DAG.computeKnownBits(Op1).isNegative();
bool AbsOpInSignedRange = DAG.SignBitIsZero(Op0) &&
(IsAdd ? Op1SignBitIsOne: DAG.SignBitIsZero(Op1));
```
The condition should be inverted.
…On August 14, 2026 5:57:48 PM GMT+02:00, Craig Topper ***@***.***> wrote:
@topperc approved this pull request.
LGTM
--
Reply to this email directly or view it on GitHub:
#215548 (review)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
I'm a bit confused: at the moment, the patch doesn't affect what happens when |
My comments were just trying to understand the history. The IsAdd code was removed in a later commit but left the subtract broken due to the inverted conditional that existed when add was removed. |
|
/cherry-pick 93030c3 |
Error: Command failed due to missing milestone. |
github-project-automation
Bot
moved this from Needs Triage
to Done
in LLVM Release Status
|
/cherry-pick 93030c3 |
|
/pull-request #217203 |
…#215548) The fold here for (abs (sub x y)) -> (abdu x y) was proven in Alive, assuming that both operands had a sign bit of zero. However, the code was checking if x had a sign bit of zero and y had a sign bit of 1 Fixes llvm#214942 Original Alive proof from llvm#186659 : https://alive2.llvm.org/ce/z/HfPF5q A variant that's explicitly (abs (sub x y)): https://alive2.llvm.org/ce/z/QEgDaa And changing the range to 32770 or higher there will break the transformation (cherry picked from commit 93030c3)
4 participants
The fold here for (abs (sub x y)) -> (abdu x y) was proven in Alive, assuming that both operands had a sign bit of zero. However, the code was checking if x had a sign bit of zero and y had a sign bit of 1
Fixes #214942
Original Alive proof from #186659 : https://alive2.llvm.org/ce/z/HfPF5q
A variant that's explicitly (abs (sub x y)): https://alive2.llvm.org/ce/z/QEgDaa
And changing the range to 32770 or higher there will break the transformation
See also previously: #196782 which seemed to fix a variant of this in the abs(add) path
Also, I assume this will want to be cherry-picked for the 23.x release branch since it's a regression from 22.x
cc @DaKnig