[AArch64] Signed comparison using CMN is safe when the subtraction is nsw #141993
Conversation
|
@llvm/pr-subscribers-backend-aarch64 Author: AZero13 (AZero13) Changesnsw means no signed wrap, and 0 - INT_MIN is a signed wrap. Now, this is going to be a point I need to get out of the way: So is it okay to always transform a > -b into cmn if it is a signed comparison, even if b is INT_MIN because -INT_MIN is undefined, at least in C. Taking advantage of UB doesn't introduce bugs. But, this could change behavior in programs with UB. Full diff: https://github.com/llvm/llvm-project/pull/141993.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
index a07afea963e20..27b612053cfd9 100644
--- a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
@@ -3337,8 +3337,13 @@ static bool isLegalArithImmed(uint64_t C) {
return IsLegal;
}
-static bool cannotBeIntMin(SDValue CheckedVal, SelectionDAG &DAG) {
- KnownBits KnownSrc = DAG.computeKnownBits(CheckedVal);
+static bool cannotBeIntMin(SDValue Op, SelectionDAG &DAG) {
+ // 0 - INT_MIN sign wraps, so no signed wrap means cmn.
+ if (Op->getFlags().hasNoSignedWrap())
+ return true;
+
+ // Maybe nsw was not set here...
+ KnownBits KnownSrc = DAG.computeKnownBits(Op.getOperand(1));
return !KnownSrc.getSignedMinValue().isMinSignedValue();
}
@@ -3356,7 +3361,7 @@ static bool isCMN(SDValue Op, ISD::CondCode CC, SelectionDAG &DAG) {
return Op.getOpcode() == ISD::SUB && isNullConstant(Op.getOperand(0)) &&
(isIntEqualitySetCC(CC) ||
(isUnsignedIntSetCC(CC) && DAG.isKnownNeverZero(Op.getOperand(1))) ||
- (isSignedIntSetCC(CC) && cannotBeIntMin(Op.getOperand(1), DAG)));
+ (isSignedIntSetCC(CC) && cannotBeIntMin(Op, DAG)));
}
static SDValue emitStrictFPComparison(SDValue LHS, SDValue RHS, const SDLoc &dl,
diff --git a/llvm/test/CodeGen/AArch64/cmp-to-cmn.ll b/llvm/test/CodeGen/AArch64/cmp-to-cmn.ll
index e87d43161a895..7a3bbc3307461 100644
--- a/llvm/test/CodeGen/AArch64/cmp-to-cmn.ll
+++ b/llvm/test/CodeGen/AArch64/cmp-to-cmn.ll
@@ -430,3 +430,49 @@ entry:
%cmp = icmp ne i32 %conv, %add
ret i1 %cmp
}
+
+define i1 @cmn_nsw(i32 %a, i32 %b) {
+; CHECK-LABEL: cmn_nsw:
+; CHECK: // %bb.0:
+; CHECK-NEXT: cmn w0, w1
+; CHECK-NEXT: cset w0, gt
+; CHECK-NEXT: ret
+%sub = sub nsw i32 0, %b
+%cmp = icmp sgt i32 %a, %sub
+ret i1 %cmp
+}
+
+define i1 @cmn_nsw_64(i64 %a, i64 %b) {
+; CHECK-LABEL: cmn_nsw_64:
+; CHECK: // %bb.0:
+; CHECK-NEXT: cmn x0, x1
+; CHECK-NEXT: cset w0, gt
+; CHECK-NEXT: ret
+%sub = sub nsw i64 0, %b
+%cmp = icmp sgt i64 %a, %sub
+ret i1 %cmp
+}
+
+define i1 @cmn_nsw_neg(i32 %a, i32 %b) {
+; CHECK-LABEL: cmn_nsw_neg:
+; CHECK: // %bb.0:
+; CHECK-NEXT: neg w8, w1
+; CHECK-NEXT: cmp w0, w8
+; CHECK-NEXT: cset w0, gt
+; CHECK-NEXT: ret
+%sub = sub i32 0, %b
+%cmp = icmp sgt i32 %a, %sub
+ret i1 %cmp
+}
+
+define i1 @cmn_nsw_neg_64(i64 %a, i64 %b) {
+; CHECK-LABEL: cmn_nsw_neg_64:
+; CHECK: // %bb.0:
+; CHECK-NEXT: neg x8, x1
+; CHECK-NEXT: cmp x0, x8
+; CHECK-NEXT: cset w0, gt
+; CHECK-NEXT: ret
+%sub = sub i64 0, %b
+%cmp = icmp sgt i64 %a, %sub
+ret i1 %cmp
+}
|
|
When I remove the nsw check, tests fail, but for all those tests, alive2 says adding nsw to the sub causes no changes? |
|
Is it possible for a sub node to be made without nsw by mistake? |
I guessing the tests that are failing are handwritten without consideration of the |
|
Alright so let's keep the KnownBits for now |
Yeah that would be me. I wrote these tests without realizing I could use nsw. |
AZero13
force-pushed
the
cmn-what-is-it
branch
3 times, most recently
from
bfec5a3 to
60ceb0e
Compare
|
@davemgreen Thoughts on this? |
|
Yeah, we create SUB nodes and they do not carry that nsw information so we need to keep the knownzero check for now. |
We make SUB nodes in dagcombine, and they do not carry flags, so this check is still good. |
|
@topperc ping |
|
@topperc Okay, I fixed it, and I learned my lesson. No force-pushing. Just a new commit. |
|
I am. Let's do this @davemgreen |
… nsw nsw means no signed wrap, and 0 - INT_MIN is a signed wrap. Now, this is going to be a point I need to get out of the way: So is it okay to always transform a > -b into cmn if it is a signed comparison, even if b is INT_MIN because -INT_MIN is undefined, at least in C, because unless fwrapv is specified, opt puts nsw on signed integer operations, allowing for more folds anyway.
|
I fixed a comment typo, build failed for unrelated reason, so I just rebased just to ensure everything is good. Everything is good. We can merge. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/30652 Here is the relevant piece of the build log for the reference |
… nsw (llvm#141993) nsw means no signed wrap, and 0 - INT_MIN is a signed wrap. Now, this is going to be a point I need to get out of the way: So is it okay to always transform a > -b into cmn if it is a signed comparison, even if b is INT_MIN because -INT_MIN is undefined, at least in C, because unless fwrapv is specified, opt puts nsw on signed integer operations, allowing for more folds anyway.
nsw means no signed wrap, and 0 - INT_MIN is a signed wrap.
Now, this is going to be a point I need to get out of the way:
So is it okay to always transform a > -b into cmn if it is a signed comparison, even if b is INT_MIN because -INT_MIN is undefined, at least in C, because unless fwrapv is specified, opt puts nsw on signed integer operations, allowing for more folds anyway.