[AArch64] Fold swapped sub/SUBS conditions #121412
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-aarch64 Author: David Green (davemgreen) ChangesThis fold already exists in a couple places (DAG and CGP), where an icmps operands are swapped to allow CSE with a sub. They do not handle constants though. This patch adds an AArch64 version that can be more precise. Full diff: https://github.com/llvm/llvm-project/pull/121412.diff 3 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
index 24e1ebd8421fbf..970250d8fb766f 100644
--- a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
@@ -24854,6 +24854,30 @@ static SDValue performCSELCombine(SDNode *N,
if (SDValue Folded = foldCSELofCTTZ(N, DAG))
return Folded;
+ // CSEL a, b, cc, SUBS(x, y) -> CSEL a, b, swapped(cc), SUBS(y, x)
+ // if SUB(y, x) already exists and we can produce a swapped predicate for cc.
+ SDValue Cond = N->getOperand(3);
+ if (DCI.isAfterLegalizeDAG() && Cond.getOpcode() == AArch64ISD::SUBS &&
+ Cond.hasOneUse() && Cond->hasNUsesOfValue(0, 0) &&
+ DAG.doesNodeExist(ISD::SUB, N->getVTList(),
+ {Cond.getOperand(1), Cond.getOperand(0)}) &&
+ !DAG.doesNodeExist(ISD::SUB, N->getVTList(),
+ {Cond.getOperand(0), Cond.getOperand(1)}) &&
+ !isNullConstant(Cond.getOperand(1))) {
+ AArch64CC::CondCode OldCond =
+ static_cast<AArch64CC::CondCode>(N->getConstantOperandVal(2));
+ AArch64CC::CondCode NewCond = getSwappedCondition(OldCond);
+ if (NewCond != AArch64CC::AL) {
+ SDLoc DL(N);
+ SDValue Sub = DAG.getNode(AArch64ISD::SUBS, DL, Cond->getVTList(),
+ Cond.getOperand(1), Cond.getOperand(0));
+ return DAG.getNode(AArch64ISD::CSEL, DL, N->getVTList(), N->getOperand(0),
+ N->getOperand(1),
+ DAG.getConstant(NewCond, DL, MVT::i32),
+ Sub.getValue(1));
+ }
+ }
+
return performCONDCombine(N, DCI, DAG, 2, 3);
}
diff --git a/llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h b/llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h
index e0ccba4d6a59e8..39d83e61705f00 100644
--- a/llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h
+++ b/llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h
@@ -306,6 +306,36 @@ inline static CondCode getInvertedCondCode(CondCode Code) {
return static_cast<CondCode>(static_cast<unsigned>(Code) ^ 0x1);
}
+/// getSwappedCondition - assume the flags are set by MI(a,b), return
+/// the condition code if we modify the instructions such that flags are
+/// set by MI(b,a).
+inline static CondCode getSwappedCondition(CondCode CC) {
+ switch (CC) {
+ default:
+ return AL;
+ case EQ:
+ return EQ;
+ case NE:
+ return NE;
+ case HS:
+ return LS;
+ case LO:
+ return HI;
+ case HI:
+ return LO;
+ case LS:
+ return HS;
+ case GE:
+ return LE;
+ case LT:
+ return GT;
+ case GT:
+ return LT;
+ case LE:
+ return GE;
+ }
+}
+
/// Given a condition code, return NZCV flags that would satisfy that condition.
/// The flag bits are in the format expected by the ccmp instructions.
/// Note that many different flag settings can satisfy a given condition code,
diff --git a/llvm/test/CodeGen/AArch64/csel-subs-swapped.ll b/llvm/test/CodeGen/AArch64/csel-subs-swapped.ll
index 7c628cf1683d6f..3971da27cdddca 100644
--- a/llvm/test/CodeGen/AArch64/csel-subs-swapped.ll
+++ b/llvm/test/CodeGen/AArch64/csel-subs-swapped.ll
@@ -5,8 +5,7 @@ define i32 @eq_i32(i32 %x) {
; CHECK-LABEL: eq_i32:
; CHECK: // %bb.0:
; CHECK-NEXT: mov w8, #-2097152 // =0xffe00000
-; CHECK-NEXT: cmn w0, #512, lsl #12 // =2097152
-; CHECK-NEXT: sub w8, w8, w0
+; CHECK-NEXT: subs w8, w8, w0
; CHECK-NEXT: csel w0, w0, w8, eq
; CHECK-NEXT: ret
%cmp = icmp eq i32 %x, -2097152
@@ -19,8 +18,7 @@ define i32 @ne_i32(i32 %x) {
; CHECK-LABEL: ne_i32:
; CHECK: // %bb.0:
; CHECK-NEXT: mov w8, #-2097152 // =0xffe00000
-; CHECK-NEXT: cmn w0, #512, lsl #12 // =2097152
-; CHECK-NEXT: sub w8, w8, w0
+; CHECK-NEXT: subs w8, w8, w0
; CHECK-NEXT: csel w0, w0, w8, ne
; CHECK-NEXT: ret
%cmp = icmp ne i32 %x, -2097152
@@ -33,9 +31,8 @@ define i32 @sgt_i32(i32 %x) {
; CHECK-LABEL: sgt_i32:
; CHECK: // %bb.0:
; CHECK-NEXT: mov w8, #-2097152 // =0xffe00000
-; CHECK-NEXT: cmn w0, #512, lsl #12 // =2097152
-; CHECK-NEXT: sub w8, w8, w0
-; CHECK-NEXT: csel w0, w0, w8, gt
+; CHECK-NEXT: subs w8, w8, w0
+; CHECK-NEXT: csel w0, w0, w8, lt
; CHECK-NEXT: ret
%cmp = icmp sgt i32 %x, -2097152
%sub = sub i32 -2097152, %x
@@ -62,9 +59,8 @@ define i32 @slt_i32(i32 %x) {
; CHECK-LABEL: slt_i32:
; CHECK: // %bb.0:
; CHECK-NEXT: mov w8, #-2097152 // =0xffe00000
-; CHECK-NEXT: cmn w0, #512, lsl #12 // =2097152
-; CHECK-NEXT: sub w8, w8, w0
-; CHECK-NEXT: csel w0, w0, w8, lt
+; CHECK-NEXT: subs w8, w8, w0
+; CHECK-NEXT: csel w0, w0, w8, gt
; CHECK-NEXT: ret
%cmp = icmp slt i32 %x, -2097152
%sub = sub i32 -2097152, %x
@@ -91,9 +87,8 @@ define i32 @ugt_i32(i32 %x) {
; CHECK-LABEL: ugt_i32:
; CHECK: // %bb.0:
; CHECK-NEXT: mov w8, #-2097152 // =0xffe00000
-; CHECK-NEXT: cmn w0, #512, lsl #12 // =2097152
-; CHECK-NEXT: sub w8, w8, w0
-; CHECK-NEXT: csel w0, w0, w8, hi
+; CHECK-NEXT: subs w8, w8, w0
+; CHECK-NEXT: csel w0, w0, w8, lo
; CHECK-NEXT: ret
%cmp = icmp ugt i32 %x, -2097152
%sub = sub i32 -2097152, %x
@@ -120,9 +115,8 @@ define i32 @ult_i32(i32 %x) {
; CHECK-LABEL: ult_i32:
; CHECK: // %bb.0:
; CHECK-NEXT: mov w8, #-2097152 // =0xffe00000
-; CHECK-NEXT: cmn w0, #512, lsl #12 // =2097152
-; CHECK-NEXT: sub w8, w8, w0
-; CHECK-NEXT: csel w0, w0, w8, lo
+; CHECK-NEXT: subs w8, w8, w0
+; CHECK-NEXT: csel w0, w0, w8, hi
; CHECK-NEXT: ret
%cmp = icmp ult i32 %x, -2097152
%sub = sub i32 -2097152, %x
@@ -150,8 +144,7 @@ define i64 @eq_i64(i64 %x) {
; CHECK-LABEL: eq_i64:
; CHECK: // %bb.0:
; CHECK-NEXT: mov w8, #100 // =0x64
-; CHECK-NEXT: cmp x0, #100
-; CHECK-NEXT: sub x8, x8, x0
+; CHECK-NEXT: subs x8, x8, x0
; CHECK-NEXT: csel x0, x0, x8, eq
; CHECK-NEXT: ret
%cmp = icmp eq i64 %x, 100
@@ -164,8 +157,7 @@ define i64 @ne_i64(i64 %x) {
; CHECK-LABEL: ne_i64:
; CHECK: // %bb.0:
; CHECK-NEXT: mov w8, #100 // =0x64
-; CHECK-NEXT: cmp x0, #100
-; CHECK-NEXT: sub x8, x8, x0
+; CHECK-NEXT: subs x8, x8, x0
; CHECK-NEXT: csel x0, x0, x8, ne
; CHECK-NEXT: ret
%cmp = icmp ne i64 %x, 100
@@ -178,9 +170,8 @@ define i64 @sgt_i64(i64 %x) {
; CHECK-LABEL: sgt_i64:
; CHECK: // %bb.0:
; CHECK-NEXT: mov w8, #100 // =0x64
-; CHECK-NEXT: cmp x0, #100
-; CHECK-NEXT: sub x8, x8, x0
-; CHECK-NEXT: csel x0, x0, x8, gt
+; CHECK-NEXT: subs x8, x8, x0
+; CHECK-NEXT: csel x0, x0, x8, lt
; CHECK-NEXT: ret
%cmp = icmp sgt i64 %x, 100
%sub = sub i64 100, %x
@@ -206,9 +197,8 @@ define i64 @slt_i64(i64 %x) {
; CHECK-LABEL: slt_i64:
; CHECK: // %bb.0:
; CHECK-NEXT: mov w8, #100 // =0x64
-; CHECK-NEXT: cmp x0, #100
-; CHECK-NEXT: sub x8, x8, x0
-; CHECK-NEXT: csel x0, x0, x8, lt
+; CHECK-NEXT: subs x8, x8, x0
+; CHECK-NEXT: csel x0, x0, x8, gt
; CHECK-NEXT: ret
%cmp = icmp slt i64 %x, 100
%sub = sub i64 100, %x
@@ -234,9 +224,8 @@ define i64 @ugt_i64(i64 %x) {
; CHECK-LABEL: ugt_i64:
; CHECK: // %bb.0:
; CHECK-NEXT: mov w8, #100 // =0x64
-; CHECK-NEXT: cmp x0, #100
-; CHECK-NEXT: sub x8, x8, x0
-; CHECK-NEXT: csel x0, x0, x8, hi
+; CHECK-NEXT: subs x8, x8, x0
+; CHECK-NEXT: csel x0, x0, x8, lo
; CHECK-NEXT: ret
%cmp = icmp ugt i64 %x, 100
%sub = sub i64 100, %x
@@ -262,9 +251,8 @@ define i64 @ult_i64(i64 %x) {
; CHECK-LABEL: ult_i64:
; CHECK: // %bb.0:
; CHECK-NEXT: mov w8, #100 // =0x64
-; CHECK-NEXT: cmp x0, #100
-; CHECK-NEXT: sub x8, x8, x0
-; CHECK-NEXT: csel x0, x0, x8, lo
+; CHECK-NEXT: subs x8, x8, x0
+; CHECK-NEXT: csel x0, x0, x8, hi
; CHECK-NEXT: ret
%cmp = icmp ult i64 %x, 100
%sub = sub i64 100, %x
|
SamTebbs33
approved these changes
Jan 6, 2025
david-arm
reviewed
Jan 6, 2025
| @@ -24854,6 +24854,30 @@ static SDValue performCSELCombine(SDNode *N, | |||
| if (SDValue Folded = foldCSELofCTTZ(N, DAG)) | |||
| return Folded; | |||
|
|
|||
| // CSEL a, b, cc, SUBS(x, y) -> CSEL a, b, swapped(cc), SUBS(y, x) | |||
| // if SUB(y, x) already exists and we can produce a swapped predicate for cc. | |||
There was a problem hiding this comment.
nit: I think this should read if SUBS(y,x) already ...
There was a problem hiding this comment.
It should be SUB(y,x). The SUBS is really a cmp from a icmp, and this commons the two.
| // if SUB(y, x) already exists and we can produce a swapped predicate for cc. | ||
| SDValue Cond = N->getOperand(3); | ||
| if (DCI.isAfterLegalizeDAG() && Cond.getOpcode() == AArch64ISD::SUBS && | ||
| Cond.hasOneUse() && Cond->hasNUsesOfValue(0, 0) && |
There was a problem hiding this comment.
I must admit I'm a bit confused about how this leads to better code because we still have to generate a brand new SUBS and the doesNotExist code is looking for a standard ISD::SUB rather than a AArch64ISD::SUBS. Is this relying upon some behaviour where we end up lowering ISD::SUB to AArch64ISD::SUBS and CSE then spots your new SUBS and the old SUBS are the same and reuses the original SUBS? If so, what if the pre-existing SUB DAG node gets optimised away or converted into a different node? I suppose the argument may be that it's at least no worse than before.
There was a problem hiding this comment.
Yes it should not be any worse, and only happen late (after legalization). We could try and use CombineTo but it didn't lead to better result when I tried it, and we have some other SUBS combines that could do with this. Considering this transform already happens in a number of other places that are even more speculative (CodeGenPrepare::swapICmpOperandsToExposeCSEOpportunities and TargetLowering::SimplifySetCC), I think it is OK.
mskamp
added a commit
to mskamp/llvm-project
that referenced
this pull request
Jan 17, 2025
This implementation has been taken from PR llvm#121412.
This fold already exists in a couple places (DAG and CGP), where an icmps operands are swapped to allow CSE with a sub. They do not handle constants though. This patch adds an AArch64 version that can be more precise.
davemgreen
force-pushed
the
gh-a64-cselsubs
branch
from
4bd0e47 to
b0776be
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/6949 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fold already exists in a couple places (DAG and CGP), where an icmps operands are swapped to allow CSE with a sub. They do not handle constants though. This patch adds an AArch64 version that can be more precise.