Retry "[SDAG] (abs (add nsw a, -b)) -> (abds a, b) (#175801)" - #186659
Conversation
|
@llvm/pr-subscribers-llvm-selectiondag @llvm/pr-subscribers-backend-x86 Author: None (DaKnig) ChangesA better version of #175801 . see that for more info. Fixes #185467 . The original patch was checking the correctness of the transformation based on the original Op1 , which was then negated (in the case of IsAdd). This patch fixes that issue by inverting the sign bit in that case. Also pushed a slight nfc there to simplify the code and remove some duplication. alive2 proofs: Full diff: https://github.com/llvm/llvm-project/pull/186659.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index 0f4503ae27998..783a060301b89 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -11847,10 +11847,29 @@ SDValue DAGCombiner::foldABSToABD(SDNode *N, const SDLoc &DL) {
EVT VT = N->getValueType(0);
SDValue Op0, Op1;
- if (!sd_match(N, m_Abs(m_Sub(m_Value(Op0), m_Value(Op1)))))
+ if (!sd_match(N, m_Abs(m_AnyOf(m_Sub(m_Value(Op0), m_Value(Op1)),
+ m_Add(m_Value(Op0), m_Value(Op1))))))
return SDValue();
SDValue AbsOp0 = N->getOperand(0);
+ bool IsAdd = AbsOp0.getOpcode() == ISD::ADD;
+ // Make sure (neg B) is positive.
+ if (IsAdd) {
+ // Elements of Op1 must be constant and != VT.minSignedValue() (or undef)
+ std::function<bool(ConstantSDNode *)> IsNotMinSignedInt =
+ [VT](ConstantSDNode *C) {
+ if (C == nullptr)
+ return true;
+ return !C->getAPIntValue()
+ .trunc(VT.getScalarSizeInBits())
+ .isMinSignedValue();
+ };
+
+ if (!ISD::matchUnaryPredicate(Op1, IsNotMinSignedInt, /*AllowUndefs=*/true,
+ /*AllowTruncation=*/true))
+ return SDValue();
+ }
+
unsigned Opc0 = Op0.getOpcode();
// Check if the operands of the sub are (zero|sign)-extended, otherwise
@@ -11858,23 +11877,43 @@ SDValue DAGCombiner::foldABSToABD(SDNode *N, const SDLoc &DL) {
if (Opc0 != Op1.getOpcode() ||
(Opc0 != ISD::ZERO_EXTEND && Opc0 != ISD::SIGN_EXTEND &&
Opc0 != ISD::SIGN_EXTEND_INREG)) {
+
+ auto CreateZextedAbd = [&](unsigned AbdOpc) {
+ if (IsAdd)
+ Op1 = DAG.getNegative(Op1, SDLoc(Op1), VT);
+ SDValue ABD = DAG.getNode(AbdOpc, DL, VT, Op0, Op1);
+ return DAG.getZExtOrTrunc(ABD, DL, SrcVT);
+ };
+
// fold (abs (sub nsw x, y)) -> abds(x, y)
+ // fold (abs (add nsw x, -y)) -> abds(x, y)
+ bool AbsOpWillNSW =
+ AbsOp0->getFlags().hasNoSignedWrap() ||
+ (IsAdd ? DAG.willNotOverflowAdd(/*IsSigned=*/true, Op0, Op1)
+ : DAG.willNotOverflowSub(/*IsSigned=*/true, Op0, Op1));
+
// Don't fold this for unsupported types as we lose the NSW handling.
if (hasOperation(ISD::ABDS, VT) && TLI.preferABDSToABSWithNSW(VT) &&
- (AbsOp0->getFlags().hasNoSignedWrap() ||
- DAG.willNotOverflowSub(/*IsSigned=*/true, Op0, Op1))) {
- SDValue ABD = DAG.getNode(ISD::ABDS, DL, VT, Op0, Op1);
- return DAG.getZExtOrTrunc(ABD, DL, SrcVT);
- }
+ AbsOpWillNSW)
+ return CreateZextedAbd(ISD::ABDS);
+
// fold (abs (sub x, y)) -> abdu(x, y)
- if (hasOperation(ISD::ABDU, VT) && DAG.SignBitIsZero(Op0) &&
- DAG.SignBitIsZero(Op1)) {
- SDValue ABD = DAG.getNode(ISD::ABDU, DL, VT, Op0, Op1);
- return DAG.getZExtOrTrunc(ABD, DL, SrcVT);
- }
+ // fold (abs (add x, -y)) -> abdu(x, y)
+ bool Op1SignBitIsOne = DAG.computeKnownBits(Op1).countMinLeadingOnes() > 0;
+ bool AbsOpWillNUW = DAG.SignBitIsZero(Op0) &&
+ (IsAdd ? DAG.SignBitIsZero(Op1) : Op1SignBitIsOne);
+
+ if (hasOperation(ISD::ABDU, VT) && AbsOpWillNUW)
+ return CreateZextedAbd(ISD::ABDU);
+
return SDValue();
}
+ // The IsAdd case explicitly checks for const/bv-of-const. This implies either
+ // (Opc0 != Op1.getOpcode() || Opc0 is not in {zext/sext/sign_ext_inreg}. This
+ // implies it was alrady handled by the above if statement.
+ assert(!IsAdd && "Unexpected abs(add(x,y)) pattern");
+
EVT VT0, VT1;
if (Opc0 == ISD::SIGN_EXTEND_INREG) {
VT0 = cast<VTSDNode>(Op0.getOperand(1))->getVT();
diff --git a/llvm/test/CodeGen/AArch64/neon-abd.ll b/llvm/test/CodeGen/AArch64/neon-abd.ll
index 98833d36dbb91..931963ee0e1e5 100644
--- a/llvm/test/CodeGen/AArch64/neon-abd.ll
+++ b/llvm/test/CodeGen/AArch64/neon-abd.ll
@@ -744,6 +744,60 @@ entry:
ret <8 x i32> %r
}
+define <4 x i32> @abs_sub(<4 x i32> %a, <4 x i32> %b) {
+; CHECK-LABEL: abs_sub:
+; CHECK: // %bb.0: // %entry
+; CHECK-NEXT: sabd v0.4s, v1.4s, v0.4s
+; CHECK-NEXT: ret
+entry:
+ %add = sub nsw <4 x i32> %b, %a
+ %cmp.i = icmp slt <4 x i32> %add, zeroinitializer
+ %sub.i = sub nsw <4 x i32> zeroinitializer, %add
+ %cond.i = select <4 x i1> %cmp.i, <4 x i32> %sub.i, <4 x i32> %add
+ ret <4 x i32> %cond.i
+}
+
+; short abs_diff_add_i16_rir(short a, short c) {
+; return abs(a - 0x492) + c;
+; }
+define <4 x i16> @abs_diff_add_v4i16(<4 x i16> %a, <4 x i16> %c) {
+; CHECK-LABEL: abs_diff_add_v4i16:
+; CHECK: // %bb.0: // %entry
+; CHECK-NEXT: mov w8, #1170 // =0x492
+; CHECK-NEXT: dup v2.4h, w8
+; CHECK-NEXT: saba v1.4h, v0.4h, v2.4h
+; CHECK-NEXT: fmov d0, d1
+; CHECK-NEXT: ret
+entry:
+ %conv = sext <4 x i16> %a to <4 x i32>
+ %sub = add nsw <4 x i32> %conv, splat(i32 -1170)
+ %0 = tail call <4 x i32> @llvm.abs.v4i32(<4 x i32> %sub, i1 true)
+ %1 = trunc <4 x i32> %0 to <4 x i16>
+ %conv2 = add <4 x i16> %1, %c
+ ret <4 x i16> %conv2
+}
+
+; short abs_diff_add_<4 x i16>_rii(short a) {
+; return abs(a - 0x93) + 0x943;
+; }
+define <4 x i16> @abs_diff_add_v4i16_rii(<4 x i16> %a) {
+; CHECK-LABEL: abs_diff_add_v4i16_rii:
+; CHECK: // %bb.0: // %entry
+; CHECK-NEXT: mov w8, #2371 // =0x943
+; CHECK-NEXT: movi v2.4h, #147
+; CHECK-NEXT: dup v1.4h, w8
+; CHECK-NEXT: saba v1.4h, v0.4h, v2.4h
+; CHECK-NEXT: fmov d0, d1
+; CHECK-NEXT: ret
+entry:
+ %conv = sext <4 x i16> %a to <4 x i32>
+ %sub = add nsw <4 x i32> %conv, splat(i32 -147)
+ %0 = tail call <4 x i32> @llvm.abs.v4i32(<4 x i32> %sub, i1 true)
+ %1 = trunc <4 x i32> %0 to <4 x i16>
+ %conv1 = add nuw <4 x i16> %1, splat(i16 2371)
+ ret <4 x i16> %conv1
+}
+
declare <8 x i8> @llvm.abs.v8i8(<8 x i8>, i1)
declare <16 x i8> @llvm.abs.v16i8(<16 x i8>, i1)
diff --git a/llvm/test/CodeGen/X86/abds.ll b/llvm/test/CodeGen/X86/abds.ll
index a1a4ba81ae493..c18de18c174a5 100644
--- a/llvm/test/CodeGen/X86/abds.ll
+++ b/llvm/test/CodeGen/X86/abds.ll
@@ -1364,6 +1364,31 @@ define i128 @abd_select_i128(i128 %a, i128 %b) nounwind {
ret i128 %sub
}
+; This used to be miscompiled into (abdu %v, i32:-1)
+; https://github.com/llvm/llvm-project/issues/185467
+define i32 @issue185467(i32 range(i32 0, 2147483647) %v) {
+; X86-LABEL: issue185467:
+; X86: # %bb.0:
+; X86-NEXT: movl {{[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: issue185467:
+; X64: # %bb.0:
+; X64-NEXT: # kill: def $edi killed $edi def $rdi
+; X64-NEXT: leal 1(%rdi), %ecx
+; X64-NEXT: movl %ecx, %eax
+; X64-NEXT: negl %eax
+; X64-NEXT: cmovsl %ecx, %eax
+; X64-NEXT: retq
+ %v1 = add i32 %v, 1
+ %absx = call i32 @llvm.abs.i32(i32 %v1, i1 false)
+ ret i32 %absx
+}
+
declare i8 @llvm.abs.i8(i8, i1)
declare i16 @llvm.abs.i16(i16, i1)
declare i32 @llvm.abs.i32(i32, i1)
|
|
@RKSimon pls review :) |
|
ping |
| if (IsAdd) { | ||
| // Elements of Op1 must be constant and != VT.minSignedValue() (or undef) | ||
| std::function<bool(ConstantSDNode *)> IsNotMinSignedInt = | ||
| [VT](ConstantSDNode *C) { |
There was a problem hiding this comment.
(style) this is one of the few times we tend to use auto:
auto IsNotMinSignedInt = [VT](ConstantSDNode *C) {
| @@ -1364,6 +1364,31 @@ define i128 @abd_select_i128(i128 %a, i128 %b) nounwind { | |||
| ret i128 %sub | |||
| } | |||
|
|
|||
| ; This used to be miscompiled into (abdu %v, i32:-1) | |||
| ; https://github.com/llvm/llvm-project/issues/185467 | |||
| define i32 @issue185467(i32 range(i32 0, 2147483647) %v) { | |||
There was a problem hiding this comment.
| define i32 @issue185467(i32 range(i32 0, 2147483647) %v) { | |
| define i32 @PR185467(i32 range(i32 0, 2147483647) %v) { |
| ; CHECK: // %bb.0: // %entry | ||
| ; CHECK-NEXT: sabd v0.4s, v1.4s, v0.4s | ||
| ; CHECK-NEXT: ret | ||
| entry: |
There was a problem hiding this comment.
(style) remove entry labels ((and rename numbered variables)
There was a problem hiding this comment.
I have updated the whole file, applied this change to other unrelated tests too, but it does not introduce that much noise so unless there's a strong objection, lets keep it in ( I know it is not good practice etc...)
| if (C == nullptr) | ||
| return true; | ||
| return !C->getAPIntValue() | ||
| .trunc(VT.getScalarSizeInBits()) |
There was a problem hiding this comment.
Is the trunc necessary? Wouldn't C already have the correct width?
There was a problem hiding this comment.
when N is TRUNC (see comment above the function), then Op1, Op2 might be of a different type.
| return SDValue(); | ||
|
|
||
| SDValue AbsOp0 = N->getOperand(0); | ||
| bool IsAdd = AbsOp0.getOpcode() == ISD::ADD; | ||
| // Make sure (neg B) is positive. |
There was a problem hiding this comment.
The comment doesn't seem right. You're only avoiding INT_MIN. Did you mean make sure (abs B) is positive?
There was a problem hiding this comment.
you are absolutely right
| return DAG.getZExtOrTrunc(ABD, DL, SrcVT); | ||
| } | ||
| // fold (abs (add x, -y)) -> abdu(x, y) | ||
| bool Op1SignBitIsOne = DAG.computeKnownBits(Op1).countMinLeadingOnes() > 0; |
There was a problem hiding this comment.
Isn't this KnownBits::isNegative()?
| bool Op1SignBitIsOne = DAG.computeKnownBits(Op1).countMinLeadingOnes() > 0; | |
| bool Op1SignBitIsOne = DAG.computeKnownBits(Op1).isNegative(); |
This is beneficial for bv of constants. alive2: https://alive2.llvm.org/ce/z/e3GsWZ
…vm#186659) A better version of llvm#175801 . see that for more info. Fixes llvm#185467 The original patch was checking the correctness of the transformation based on the original Op1 , which was then negated (in the case of IsAdd). This patch fixes that issue by inverting the sign bit in that case. Also pushed a slight nfc there to simplify the code and remove some duplication. alive2 proofs: abds: https://alive2.llvm.org/ce/z/oJQPss abdu: https://alive2.llvm.org/ce/z/HfPF5q Note that the regression test is not (wrongly) affected anymore by the patch (as it did before)
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`. Related: #185467 #175801
…96782) The abs(add(x, y)) → abdu(x, -y) fold added in llvm#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`. Related: llvm#185467 llvm#175801
…vm#186659) A better version of llvm#175801 . see that for more info. Fixes llvm#185467 The original patch was checking the correctness of the transformation based on the original Op1 , which was then negated (in the case of IsAdd). This patch fixes that issue by inverting the sign bit in that case. Also pushed a slight nfc there to simplify the code and remove some duplication. alive2 proofs: abds: https://alive2.llvm.org/ce/z/oJQPss abdu: https://alive2.llvm.org/ce/z/HfPF5q Note that the regression test is not (wrongly) affected anymore by the patch (as it did before)
…96782) The abs(add(x, y)) → abdu(x, -y) fold added in llvm#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`. Related: llvm#185467 llvm#175801
) 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
… fold (#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/llvm-project#214942 Original Alive proof from llvm/llvm-project#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
… fold (#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/llvm-project#214942 Original Alive proof from llvm/llvm-project#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
…#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)
… fold (#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/llvm-project#214942 Original Alive proof from llvm/llvm-project#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)
… fold (#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/llvm-project#214942 Original Alive proof from llvm/llvm-project#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)
…548) 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/llvm-project#214942 Original Alive proof from llvm/llvm-project#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 82a436c) Signed-off-by: Hafidz Muzakky <ais.muzakky@gmail.com>
A better version of #175801 . see that for more info.
Fixes #185467 .
The original patch was checking the correctness of the transformation based on the original Op1 , which was then negated (in the case of IsAdd). This patch fixes that issue by inverting the sign bit in that case.
Also pushed a slight nfc there to simplify the code and remove some duplication.
alive2 proofs:
abds: https://alive2.llvm.org/ce/z/oJQPss
abdu: https://alive2.llvm.org/ce/z/HfPF5q
Note that the regression test is not (wrongly) affected anymore by the patch (as it did before)