-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[SelectionDAG] Lowering usub.sat(a, 1) to a - (a != 0) #170076
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-backend-risc-v Author: guan jian (rez5427) ChangesI recently observed that LLVM generates the following code: This could be optimized using the snez instruction instead. Full diff: https://github.com/llvm/llvm-project/pull/170076.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
index 521d8f07434e6..b3af59b6528b9 100644
--- a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
@@ -10867,6 +10867,16 @@ SDValue TargetLowering::expandAddSubSat(SDNode *Node, SelectionDAG &DAG) const {
assert(VT == RHS.getValueType() && "Expected operands to be the same type");
assert(VT.isInteger() && "Expected operands to be integers");
+ // usub.sat(a, 1) -> a - zext(a != 0)
+ if (Opcode == ISD::USUBSAT && !VT.isVector() && isOneConstant(RHS)) {
+ SDValue Zero = DAG.getConstant(0, dl, VT);
+ EVT BoolVT =
+ getSetCCResultType(DAG.getDataLayout(), *DAG.getContext(), VT);
+ SDValue IsNonZero = DAG.getSetCC(dl, BoolVT, LHS, Zero, ISD::SETNE);
+ SDValue Subtrahend = DAG.getBoolExtOrTrunc(IsNonZero, dl, VT, BoolVT);
+ return DAG.getNode(ISD::SUB, dl, VT, LHS, Subtrahend);
+ }
+
// usub.sat(a, b) -> umax(a, b) - b
if (Opcode == ISD::USUBSAT && isOperationLegal(ISD::UMAX, VT)) {
SDValue Max = DAG.getNode(ISD::UMAX, dl, VT, LHS, RHS);
diff --git a/llvm/test/CodeGen/AArch64/and-mask-removal.ll b/llvm/test/CodeGen/AArch64/and-mask-removal.ll
index 5046c0571ad2b..855fe5caf97b2 100644
--- a/llvm/test/CodeGen/AArch64/and-mask-removal.ll
+++ b/llvm/test/CodeGen/AArch64/and-mask-removal.ll
@@ -483,9 +483,9 @@ define i64 @pr58109(i8 signext %0) {
; CHECK-SD-LABEL: pr58109:
; CHECK-SD: ; %bb.0:
; CHECK-SD-NEXT: add w8, w0, #1
-; CHECK-SD-NEXT: and w8, w8, #0xff
-; CHECK-SD-NEXT: subs w8, w8, #1
-; CHECK-SD-NEXT: csel w0, wzr, w8, lo
+; CHECK-SD-NEXT: ands w8, w8, #0xff
+; CHECK-SD-NEXT: cset w9, ne
+; CHECK-SD-NEXT: sub w0, w8, w9
; CHECK-SD-NEXT: ret
;
; CHECK-GI-LABEL: pr58109:
diff --git a/llvm/test/CodeGen/RISCV/usub_sat.ll b/llvm/test/CodeGen/RISCV/usub_sat.ll
index 33056682dcc79..7084885a0ee2c 100644
--- a/llvm/test/CodeGen/RISCV/usub_sat.ll
+++ b/llvm/test/CodeGen/RISCV/usub_sat.ll
@@ -185,3 +185,31 @@ define zeroext i4 @func3(i4 zeroext %x, i4 zeroext %y) nounwind {
%tmp = call i4 @llvm.usub.sat.i4(i4 %x, i4 %y);
ret i4 %tmp;
}
+
+define signext i32 @sat_dec_i32(i32 signext %x) nounwind {
+; RV32I-LABEL: sat_dec_i32:
+; RV32I: # %bb.0:
+; RV32I-NEXT: snez a1, a0
+; RV32I-NEXT: sub a0, a0, a1
+; RV32I-NEXT: ret
+;
+; RV64I-LABEL: sat_dec_i32:
+; RV64I: # %bb.0:
+; RV64I-NEXT: snez a1, a0
+; RV64I-NEXT: subw a0, a0, a1
+; RV64I-NEXT: ret
+;
+; RV32IZbb-LABEL: sat_dec_i32:
+; RV32IZbb: # %bb.0:
+; RV32IZbb-NEXT: snez a1, a0
+; RV32IZbb-NEXT: sub a0, a0, a1
+; RV32IZbb-NEXT: ret
+;
+; RV64IZbb-LABEL: sat_dec_i32:
+; RV64IZbb: # %bb.0:
+; RV64IZbb-NEXT: snez a1, a0
+; RV64IZbb-NEXT: subw a0, a0, a1
+; RV64IZbb-NEXT: ret
+ %tmp = call i32 @llvm.usub.sat.i32(i32 %x, i32 1)
+ ret i32 %tmp
+}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| EVT BoolVT = | ||
| getSetCCResultType(DAG.getDataLayout(), *DAG.getContext(), VT); | ||
| SDValue IsNonZero = DAG.getSetCC(dl, BoolVT, LHS, Zero, ISD::SETNE); | ||
| SDValue Subtrahend = DAG.getBoolExtOrTrunc(IsNonZero, dl, VT, BoolVT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this assume ZeroOrOneBooleanContent? Also what is Subtrahend short for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't Subtrahend the word for the right hand side of a subtraction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I learned "minuend" and "subtrahend" from the ZX Spectrum BASIC manual :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this assume ZeroOrOneBooleanContent?
I think so. For ZeroNegativeOneBooleanContent we would need to use ADD instead of SUB.
| assert(VT.isInteger() && "Expected operands to be integers"); | ||
|
|
||
| // usub.sat(a, 1) -> a - zext(a != 0) | ||
| if (Opcode == ISD::USUBSAT && !VT.isVector() && isOneConstant(RHS)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why no vector support?
|
Another option is |
| SDValue Zero = DAG.getConstant(0, dl, VT); | ||
| EVT BoolVT = | ||
| getSetCCResultType(DAG.getDataLayout(), *DAG.getContext(), VT); | ||
| SDValue IsNonZero = DAG.getSetCC(dl, BoolVT, LHS, Zero, ISD::SETNE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to freeze LHS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That shows that it works with freeze. Looks like it fails if the freeze is removed?
| SDValue Zero = DAG.getConstant(0, dl, VT); | ||
| EVT BoolVT = getSetCCResultType(DAG.getDataLayout(), *DAG.getContext(), VT); | ||
| SDValue IsNonZero = DAG.getSetCC(dl, BoolVT, LHS, Zero, ISD::SETNE); | ||
| SDValue Subtrahend = DAG.getZExtOrTrunc(IsNonZero, dl, VT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't going to work as BoolVT might already be the same size as VT
You might have to do this:
SDValue Subtrahend = DAG.getBoolExtOrTrunc(IsNonZero, dl, VT);
Subtrahend = getNode(ISD::AND, DL, VT, Subtrahend, getConstant(1, DL, VT));
| assert(VT.isInteger() && "Expected operands to be integers"); | ||
|
|
||
| // usub.sat(a, 1) -> sub(a, zext(a != 0)) | ||
| if (Opcode == ISD::USUBSAT && isOneConstant(RHS)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isOneConstant only returns true for scalar. So this still doesn't support vector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw some regressions on some test files, I use !VT.isVector() && isOneConstant(RHS) now.
Co-authored-by: Simon Pilgrim <git@redking.me.uk>
|
Well, I rebase it, And add freeze, and limit if for scalar, because I saw some regressions on test files. |
I recently observed that LLVM generates the following code:
This could be optimized using the snez instruction instead.