[AArch64] Peek through freeze in setcc if it has one_use, and is comparing against a constant #153897
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 @llvm/pr-subscribers-llvm-selectiondag Author: AZero13 (AZero13) ChangesRather than having only brcond check, we should just allow all optimizations to see through the freeze if it is one-use. Full diff: https://github.com/llvm/llvm-project/pull/153897.diff 4 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index 17703f58f2824..f4b3b4e1c8037 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -13544,6 +13544,46 @@ SDValue DAGCombiner::visitSETCC(SDNode *N) {
SDValue N0 = N->getOperand(0), N1 = N->getOperand(1);
SDLoc DL(N);
+ // Is 'X Cond C' always true or false?
+ auto IsAlwaysTrueOrFalse = [](ISD::CondCode Cond, ConstantSDNode *C) {
+ bool False = (Cond == ISD::SETULT && C->isZero()) ||
+ (Cond == ISD::SETLT && C->isMinSignedValue()) ||
+ (Cond == ISD::SETUGT && C->isAllOnes()) ||
+ (Cond == ISD::SETGT && C->isMaxSignedValue());
+ bool True = (Cond == ISD::SETULE && C->isAllOnes()) ||
+ (Cond == ISD::SETLE && C->isMaxSignedValue()) ||
+ (Cond == ISD::SETUGE && C->isZero()) ||
+ (Cond == ISD::SETGE && C->isMinSignedValue());
+ return True || False;
+ };
+
+ // Peek through freeze if it has one use. This is safe for comparisons that
+ // are not "always true" or "always false" when the operand is poison. Unsafe
+ // cases include:
+ // - X < 0 (SETULT), X >= 0 (SETUGE) - always false/true when X is poison
+ // - X < MIN_SIGNED (SETLT), X >= MIN_SIGNED (SETGE) - always false/true when
+ // X is poison
+ // - X > MAX_UNSIGNED (SETUGT), X <= MAX_UNSIGNED (SETULE) - always false/true
+ // when X is poison
+ // - X > MAX_SIGNED (SETGT), X <= MAX_SIGNED (SETLE) - always false/true when
+ // X is poison
+ if (N0->getOpcode() == ISD::FREEZE && N0.hasOneUse()) {
+ // Check if this comparison could be "always true" or "always false" when
+ // the operand is poison, which would make peeking through freeze unsafe.
+ ConstantSDNode *N1C = dyn_cast<ConstantSDNode>(N1);
+ if (!N1C || !IsAlwaysTrueOrFalse(Cond, N1C)) {
+ N0 = N0->getOperand(0);
+ }
+ }
+ if (N1->getOpcode() == ISD::FREEZE && N1.hasOneUse()) {
+ // Check if this comparison could be "always true" or "always false" when
+ // the operand is poison, which would make peeking through freeze unsafe.
+ ConstantSDNode *N0C = dyn_cast<ConstantSDNode>(N0);
+ if (!N0C || !IsAlwaysTrueOrFalse(ISD::getSetCCSwappedOperands(Cond), N0C)) {
+ N1 = N1->getOperand(0);
+ }
+ }
+
if (SDValue Combined = SimplifySetCC(VT, N0, N1, Cond, DL, !PreferSetCC)) {
// If we prefer to have a setcc, and we don't, we'll try our best to
// recreate one using rebuildSetCC.
@@ -19287,56 +19327,6 @@ SDValue DAGCombiner::visitBRCOND(SDNode *N) {
N1->getOperand(0), N2, N->getFlags());
}
- // Variant of the previous fold where there is a SETCC in between:
- // BRCOND(SETCC(FREEZE(X), CONST, Cond))
- // =>
- // BRCOND(FREEZE(SETCC(X, CONST, Cond)))
- // =>
- // BRCOND(SETCC(X, CONST, Cond))
- // This is correct if FREEZE(X) has one use and SETCC(FREEZE(X), CONST, Cond)
- // isn't equivalent to true or false.
- // For example, SETCC(FREEZE(X), -128, SETULT) cannot be folded to
- // FREEZE(SETCC(X, -128, SETULT)) because X can be poison.
- if (N1->getOpcode() == ISD::SETCC && N1.hasOneUse()) {
- SDValue S0 = N1->getOperand(0), S1 = N1->getOperand(1);
- ISD::CondCode Cond = cast<CondCodeSDNode>(N1->getOperand(2))->get();
- ConstantSDNode *S0C = dyn_cast<ConstantSDNode>(S0);
- ConstantSDNode *S1C = dyn_cast<ConstantSDNode>(S1);
- bool Updated = false;
-
- // Is 'X Cond C' always true or false?
- auto IsAlwaysTrueOrFalse = [](ISD::CondCode Cond, ConstantSDNode *C) {
- bool False = (Cond == ISD::SETULT && C->isZero()) ||
- (Cond == ISD::SETLT && C->isMinSignedValue()) ||
- (Cond == ISD::SETUGT && C->isAllOnes()) ||
- (Cond == ISD::SETGT && C->isMaxSignedValue());
- bool True = (Cond == ISD::SETULE && C->isAllOnes()) ||
- (Cond == ISD::SETLE && C->isMaxSignedValue()) ||
- (Cond == ISD::SETUGE && C->isZero()) ||
- (Cond == ISD::SETGE && C->isMinSignedValue());
- return True || False;
- };
-
- if (S0->getOpcode() == ISD::FREEZE && S0.hasOneUse() && S1C) {
- if (!IsAlwaysTrueOrFalse(Cond, S1C)) {
- S0 = S0->getOperand(0);
- Updated = true;
- }
- }
- if (S1->getOpcode() == ISD::FREEZE && S1.hasOneUse() && S0C) {
- if (!IsAlwaysTrueOrFalse(ISD::getSetCCSwappedOperands(Cond), S0C)) {
- S1 = S1->getOperand(0);
- Updated = true;
- }
- }
-
- if (Updated)
- return DAG.getNode(
- ISD::BRCOND, SDLoc(N), MVT::Other, Chain,
- DAG.getSetCC(SDLoc(N1), N1->getValueType(0), S0, S1, Cond), N2,
- N->getFlags());
- }
-
// If N is a constant we could fold this into a fallthrough or unconditional
// branch. However that doesn't happen very often in normal code, because
// Instcombine/SimplifyCFG should have handled the available opportunities.
diff --git a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
index 2072e48914ae6..f4ca96768a6f2 100644
--- a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
@@ -3604,19 +3604,25 @@ static SDValue emitComparison(SDValue LHS, SDValue RHS, ISD::CondCode CC,
Opcode = AArch64ISD::ADDS;
LHS = LHS.getOperand(1);
} else if (isNullConstant(RHS) && !isUnsignedIntSetCC(CC)) {
- if (LHS.getOpcode() == ISD::AND) {
+ SDValue AndOp = LHS;
+ // Peek through freeze to find AND operation
+ if (LHS.getOpcode() == ISD::FREEZE && LHS.hasOneUse()) {
+ AndOp = LHS.getOperand(0);
+ }
+
+ if (AndOp.getOpcode() == ISD::AND) {
// Similarly, (CMP (and X, Y), 0) can be implemented with a TST
// (a.k.a. ANDS) except that the flags are only guaranteed to work for one
// of the signed comparisons.
const SDValue ANDSNode =
DAG.getNode(AArch64ISD::ANDS, DL, DAG.getVTList(VT, FlagsVT),
- LHS.getOperand(0), LHS.getOperand(1));
- // Replace all users of (and X, Y) with newly generated (ands X, Y)
- DAG.ReplaceAllUsesWith(LHS, ANDSNode);
+ AndOp.getOperand(0), AndOp.getOperand(1));
+ // Replace all users of the AND operation with newly generated (ands X, Y)
+ DAG.ReplaceAllUsesWith(AndOp, ANDSNode);
return ANDSNode.getValue(1);
- } else if (LHS.getOpcode() == AArch64ISD::ANDS) {
+ } else if (AndOp.getOpcode() == AArch64ISD::ANDS) {
// Use result of ANDS
- return LHS.getValue(1);
+ return AndOp.getValue(1);
}
}
diff --git a/llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp b/llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
index ee34a85a5b507..170050b934934 100644
--- a/llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
+++ b/llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
@@ -5203,15 +5203,22 @@ MachineInstr *AArch64InstructionSelector::tryFoldIntegerCompare(
// Produce this if the compare is signed:
//
// tst x, y
- if (!CmpInst::isUnsigned(P) && LHSDef &&
- LHSDef->getOpcode() == TargetOpcode::G_AND) {
+ if (!CmpInst::isUnsigned(P) && LHSDef) {
+ MachineInstr *AndDef = LHSDef;
+ // Peek through freeze to find AND operation
+ if (LHSDef->getOpcode() == TargetOpcode::G_FREEZE &&
+ MRI.hasOneUse(LHSDef->getOperand(0).getReg())) {
+ AndDef = getDefIgnoringCopies(LHSDef->getOperand(0).getReg(), MRI);
+ }
+
+ if (AndDef && AndDef->getOpcode() == TargetOpcode::G_AND) {
// Make sure that the RHS is 0.
auto ValAndVReg = getIConstantVRegValWithLookThrough(RHS.getReg(), MRI);
if (!ValAndVReg || ValAndVReg->Value != 0)
return nullptr;
- return emitTST(LHSDef->getOperand(1),
- LHSDef->getOperand(2), MIRBuilder);
+ return emitTST(AndDef->getOperand(1),
+ AndDef->getOperand(2), MIRBuilder);
}
return nullptr;
diff --git a/llvm/test/CodeGen/AArch64/icmp-ult-eq-fold.ll b/llvm/test/CodeGen/AArch64/icmp-ult-eq-fold.ll
index 33c5ba7987974..74f6f7724c390 100644
--- a/llvm/test/CodeGen/AArch64/icmp-ult-eq-fold.ll
+++ b/llvm/test/CodeGen/AArch64/icmp-ult-eq-fold.ll
@@ -161,6 +161,25 @@ define i1 @lt64_u16_and_23(i64 %0) {
ret i1 %3
}
+define i1 @test_disjoint(i1 %0, i32 %1, i32 %2) {
+; CHECK-LABEL: test_disjoint:
+; CHECK: // %bb.0: // %entry
+; CHECK-NEXT: orr w8, w2, #0x800000
+; CHECK-NEXT: lsr w8, w8, w1
+; CHECK-NEXT: tst w8, #0x1
+; CHECK-NEXT: cset w8, eq
+; CHECK-NEXT: orr w8, w0, w8
+; CHECK-NEXT: and w0, w8, #0x1
+; CHECK-NEXT: ret
+entry:
+ %3 = or disjoint i32 %2, 8388608
+ %4 = shl nuw i32 1, %1
+ %5 = and i32 %3, %4
+ %6 = icmp eq i32 %5, 0
+ %7 = select i1 %0, i1 true, i1 %6
+ ret i1 %7
+}
+
; negative test
define i1 @lt3_u8(i8 %0) {
; CHECK-LABEL: lt3_u8:
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
…nst a constant Rather than having only brcond check, we should just allow all optimizations to see through the freeze if it is one-use
|
I think brcond was treated specially because a branch on a frozen poison is non-deterministic. I don't know if you can do this on arbitrary setcc uses. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Rather than having only brcond check, we should just allow all optimizations to see through the freeze if it is one-use.