[llvm] [NewGVN] Simplify eliminateInstructions (PR #121059)
via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 26 12:29:43 PST 2024
https://github.com/ManuelJBrito updated https://github.com/llvm/llvm-project/pull/121059
>From be8ecd05a7a0438472282c5d5fc17d8346621b56 Mon Sep 17 00:00:00 2001
From: ManuelJBrito <manuel.brito at tecnico.ulisboa.pt>
Date: Tue, 24 Dec 2024 15:46:10 +0000
Subject: [PATCH 1/2] [NewGVN] Simplify eliminateInstructions
If an instruction is a dominating leader for another instruction then
it can replace all its uses. It is therefore pointless to keep uses in
the DFSOrderedSet. This patch removes that and simplifies accordingly.
The only regressions are due to not performing DCE as well as before.
Previously since we replaced use by use this allowed us to detect the
case where we where replacing into a dead user and that was the only use.
This should be fine as these instructions are trivially dead and subsequent
passes can delete them. This is something that already happens with dead phis
introduced for phi-of-ops.
---
llvm/lib/Transforms/Scalar/NewGVN.cpp | 178 ++++--------------
llvm/test/Transforms/NewGVN/commute.ll | 1 +
.../test/Transforms/NewGVN/invariant.group.ll | 1 +
.../Transforms/NewGVN/load-constant-mem.ll | 1 +
llvm/test/Transforms/NewGVN/pr31682.ll | 1 +
llvm/test/Transforms/NewGVN/tbaa.ll | 15 +-
6 files changed, 55 insertions(+), 142 deletions(-)
diff --git a/llvm/lib/Transforms/Scalar/NewGVN.cpp b/llvm/lib/Transforms/Scalar/NewGVN.cpp
index 13d9e8f186b47c..fdd67ccc86a820 100644
--- a/llvm/lib/Transforms/Scalar/NewGVN.cpp
+++ b/llvm/lib/Transforms/Scalar/NewGVN.cpp
@@ -3552,11 +3552,9 @@ struct NewGVN::ValueDFS {
int DFSOut = 0;
int LocalNum = 0;
- // Only one of Def and U will be set.
// The bool in the Def tells us whether the Def is the stored value of a
// store.
PointerIntPair<Value *, 1, bool> Def;
- Use *U = nullptr;
bool operator<(const ValueDFS &Other) const {
// It's not enough that any given field be less than - we have sets
@@ -3573,33 +3571,8 @@ struct NewGVN::ValueDFS {
// Each LLVM instruction only produces one value, and thus the lowest-level
// differentiator that really matters for the stack (and what we use as a
// replacement) is the local dfs number.
- // Everything else in the structure is instruction level, and only affects
- // the order in which we will replace operands of a given instruction.
- //
- // For a given instruction (IE things with equal dfsin, dfsout, localnum),
- // the order of replacement of uses does not matter.
- // IE given,
- // a = 5
- // b = a + a
- // When you hit b, you will have two valuedfs with the same dfsin, out, and
- // localnum.
- // The .val will be the same as well.
- // The .u's will be different.
- // You will replace both, and it does not matter what order you replace them
- // in (IE whether you replace operand 2, then operand 1, or operand 1, then
- // operand 2).
- // Similarly for the case of same dfsin, dfsout, localnum, but different
- // .val's
- // a = 5
- // b = 6
- // c = a + b
- // in c, we will a valuedfs for a, and one for b,with everything the same
- // but .val and .u.
- // It does not matter what order we replace these operands in.
- // You will always end up with the same IR, and this is guaranteed.
- return std::tie(DFSIn, DFSOut, LocalNum, Def, U) <
- std::tie(Other.DFSIn, Other.DFSOut, Other.LocalNum, Other.Def,
- Other.U);
+ return std::tie(DFSIn, DFSOut, LocalNum, Def) <
+ std::tie(Other.DFSIn, Other.DFSOut, Other.LocalNum, Other.Def);
}
};
@@ -3660,30 +3633,14 @@ void NewGVN::convertClassToDFSOrdered(
// Don't try to replace into dead uses
if (InstructionsToErase.count(I))
continue;
- ValueDFS VDUse;
// Put the phi node uses in the incoming block.
- BasicBlock *IBlock;
- if (auto *P = dyn_cast<PHINode>(I)) {
- IBlock = P->getIncomingBlock(U);
- // Make phi node users appear last in the incoming block
- // they are from.
- VDUse.LocalNum = InstrDFS.size() + 1;
- } else {
- IBlock = getBlockForValue(I);
- VDUse.LocalNum = InstrToDFSNum(I);
- }
-
+ auto *PN = dyn_cast<PHINode>(I);
+ BasicBlock *IBlock = PN ? PN->getIncomingBlock(U) : getBlockForValue(I);
// Skip uses in unreachable blocks, as we're going
// to delete them.
if (!ReachableBlocks.contains(IBlock))
continue;
-
- DomTreeNode *DomNode = DT->getNode(IBlock);
- VDUse.DFSIn = DomNode->getDFSNumIn();
- VDUse.DFSOut = DomNode->getDFSNumOut();
- VDUse.U = &U;
++UseCount;
- DFSOrderedSet.emplace_back(VDUse);
}
}
@@ -3991,7 +3948,6 @@ bool NewGVN::eliminateInstructions(Function &F) {
int MemberDFSOut = VD.DFSOut;
Value *Def = VD.Def.getPointer();
bool FromStore = VD.Def.getInt();
- Use *U = VD.U;
// We ignore void things because we can't get a value from them.
if (Def && Def->getType()->isVoidTy())
continue;
@@ -4048,100 +4004,48 @@ bool NewGVN::eliminateInstructions(Function &F) {
}
}
- // Skip the Def's, we only want to eliminate on their uses. But mark
- // dominated defs as dead.
- if (Def) {
- // For anything in this case, what and how we value number
- // guarantees that any side-effects that would have occurred (ie
- // throwing, etc) can be proven to either still occur (because it's
- // dominated by something that has the same side-effects), or never
- // occur. Otherwise, we would not have been able to prove it value
- // equivalent to something else. For these things, we can just mark
- // it all dead. Note that this is different from the "ProbablyDead"
- // set, which may not be dominated by anything, and thus, are only
- // easy to prove dead if they are also side-effect free. Note that
- // because stores are put in terms of the stored value, we skip
- // stored values here. If the stored value is really dead, it will
- // still be marked for deletion when we process it in its own class.
- auto *DefI = dyn_cast<Instruction>(Def);
- if (!EliminationStack.empty() && DefI && !FromStore) {
- Value *DominatingLeader = EliminationStack.back();
- if (DominatingLeader != Def) {
+ // For anything in this case, what and how we value number
+ // guarantees that any side-effects that would have occurred (ie
+ // throwing, etc) can be proven to either still occur (because it's
+ // dominated by something that has the same side-effects), or never
+ // occur. Otherwise, we would not have been able to prove it value
+ // equivalent to something else. For these things, we can just mark
+ // it all dead. Note that this is different from the "ProbablyDead"
+ // set, which may not be dominated by anything, and thus, are only
+ // easy to prove dead if they are also side-effect free. Note that
+ // because stores are put in terms of the stored value, we skip
+ // stored values here. If the stored value is really dead, it will
+ // still be marked for deletion when we process it in its own class.
+ auto *DefI = dyn_cast<Instruction>(Def);
+ if (!EliminationStack.empty() && DefI && !FromStore) {
+ Value *DominatingLeader = EliminationStack.back();
+ auto *II = dyn_cast<IntrinsicInst>(DominatingLeader);
+ bool isSSACopy = II && II->getIntrinsicID() == Intrinsic::ssa_copy;
+ if (isSSACopy)
+ DominatingLeader = II->getOperand(0);
+ if (DominatingLeader != Def) {
+ // All uses of Def are now uses of the dominating leader, which
+ // means if the dominating leader was dead, it's now live!
+ auto &LeaderUseCount = UseCounts[DominatingLeader];
+ if (LeaderUseCount == 0 && isa<Instruction>(DominatingLeader))
+ ProbablyDead.erase(cast<Instruction>(DominatingLeader));
+ LeaderUseCount += Def->getNumUses();
+
+ // Don't patch metadata if ssa_copy is being replaced by their
+ // original.
+ auto *PI = PredInfo->getPredicateInfoFor(DefI);
+ if (PI && DominatingLeader == PI->OriginalOp) {
+ Def->replaceAllUsesWith(DominatingLeader);
+ } else {
// Even if the instruction is removed, we still need to update
// flags/metadata due to downstreams users of the leader.
- if (!match(DefI, m_Intrinsic<Intrinsic::ssa_copy>()))
- patchReplacementInstruction(DefI, DominatingLeader);
-
- markInstructionForDeletion(DefI);
+ patchAndReplaceAllUsesWith(cast<Instruction>(Def),
+ DominatingLeader);
}
- }
- continue;
- }
- // At this point, we know it is a Use we are trying to possibly
- // replace.
-
- assert(isa<Instruction>(U->get()) &&
- "Current def should have been an instruction");
- assert(isa<Instruction>(U->getUser()) &&
- "Current user should have been an instruction");
-
- // If the thing we are replacing into is already marked to be dead,
- // this use is dead. Note that this is true regardless of whether
- // we have anything dominating the use or not. We do this here
- // because we are already walking all the uses anyway.
- Instruction *InstUse = cast<Instruction>(U->getUser());
- if (InstructionsToErase.count(InstUse)) {
- auto &UseCount = UseCounts[U->get()];
- if (--UseCount == 0) {
- ProbablyDead.insert(cast<Instruction>(U->get()));
- }
- }
-
- // If we get to this point, and the stack is empty we must have a use
- // with nothing we can use to eliminate this use, so just skip it.
- if (EliminationStack.empty())
- continue;
-
- Value *DominatingLeader = EliminationStack.back();
-
- auto *II = dyn_cast<IntrinsicInst>(DominatingLeader);
- bool isSSACopy = II && II->getIntrinsicID() == Intrinsic::ssa_copy;
- if (isSSACopy)
- DominatingLeader = II->getOperand(0);
-
- // Don't replace our existing users with ourselves.
- if (U->get() == DominatingLeader)
- continue;
- LLVM_DEBUG(dbgs()
- << "Found replacement " << *DominatingLeader << " for "
- << *U->get() << " in " << *(U->getUser()) << "\n");
-
- // If we replaced something in an instruction, handle the patching of
- // metadata. Skip this if we are replacing predicateinfo with its
- // original operand, as we already know we can just drop it.
- auto *ReplacedInst = cast<Instruction>(U->get());
- auto *PI = PredInfo->getPredicateInfoFor(ReplacedInst);
- if (!PI || DominatingLeader != PI->OriginalOp)
- patchReplacementInstruction(ReplacedInst, DominatingLeader);
- U->set(DominatingLeader);
- // This is now a use of the dominating leader, which means if the
- // dominating leader was dead, it's now live!
- auto &LeaderUseCount = UseCounts[DominatingLeader];
- // It's about to be alive again.
- if (LeaderUseCount == 0 && isa<Instruction>(DominatingLeader))
- ProbablyDead.erase(cast<Instruction>(DominatingLeader));
- // For copy instructions, we use their operand as a leader,
- // which means we remove a user of the copy and it may become dead.
- if (isSSACopy) {
- auto It = UseCounts.find(II);
- if (It != UseCounts.end()) {
- unsigned &IIUseCount = It->second;
- if (--IIUseCount == 0)
- ProbablyDead.insert(II);
+ AnythingReplaced = true;
+ markInstructionForDeletion(DefI);
}
}
- ++LeaderUseCount;
- AnythingReplaced = true;
}
}
}
diff --git a/llvm/test/Transforms/NewGVN/commute.ll b/llvm/test/Transforms/NewGVN/commute.ll
index e1622fb923716b..3ea910c685591d 100644
--- a/llvm/test/Transforms/NewGVN/commute.ll
+++ b/llvm/test/Transforms/NewGVN/commute.ll
@@ -48,6 +48,7 @@ declare i16 @llvm.umul.fix.i16(i16, i16, i32)
define i16 @intrinsic_3_args(i16 %x, i16 %y) {
; CHECK-LABEL: @intrinsic_3_args(
+; CHECK-NEXT: [[M1:%.*]] = call i16 @llvm.smul.fix.i16(i16 [[X:%.*]], i16 [[Y:%.*]], i32 1)
; CHECK-NEXT: ret i16 0
;
%m1 = call i16 @llvm.smul.fix.i16(i16 %x, i16 %y, i32 1)
diff --git a/llvm/test/Transforms/NewGVN/invariant.group.ll b/llvm/test/Transforms/NewGVN/invariant.group.ll
index 7c14059c88c677..5c2b621c296de1 100644
--- a/llvm/test/Transforms/NewGVN/invariant.group.ll
+++ b/llvm/test/Transforms/NewGVN/invariant.group.ll
@@ -74,6 +74,7 @@ entry:
define i1 @proveEqualityForStrip(ptr %a) {
; CHECK-LABEL: define i1 @proveEqualityForStrip(
; CHECK-SAME: ptr [[A:%.*]]) {
+; CHECK-NEXT: [[B1:%.*]] = call ptr @llvm.strip.invariant.group.p0(ptr [[A]])
; CHECK-NEXT: ret i1 true
;
%b1 = call ptr @llvm.strip.invariant.group.p0(ptr %a)
diff --git a/llvm/test/Transforms/NewGVN/load-constant-mem.ll b/llvm/test/Transforms/NewGVN/load-constant-mem.ll
index ae91147237fade..3362cadb1e22bd 100644
--- a/llvm/test/Transforms/NewGVN/load-constant-mem.ll
+++ b/llvm/test/Transforms/NewGVN/load-constant-mem.ll
@@ -7,6 +7,7 @@ define i32 @test(ptr %p, i32 %i) nounwind {
; CHECK-LABEL: @test(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[P:%.*]] = getelementptr [4 x i32], ptr @G, i32 0, i32 [[I:%.*]]
+; CHECK-NEXT: [[A:%.*]] = load i32, ptr [[P]], align 4
; CHECK-NEXT: store i8 4, ptr [[P1:%.*]], align 1
; CHECK-NEXT: ret i32 0
;
diff --git a/llvm/test/Transforms/NewGVN/pr31682.ll b/llvm/test/Transforms/NewGVN/pr31682.ll
index 3d8c9e28635c66..60ec91007d2db2 100644
--- a/llvm/test/Transforms/NewGVN/pr31682.ll
+++ b/llvm/test/Transforms/NewGVN/pr31682.ll
@@ -12,6 +12,7 @@ define void @bar() {
; CHECK-NEXT: [[TMP:%.*]] = load ptr, ptr @global, align 8
; CHECK-NEXT: br label [[BB2:%.*]]
; CHECK: bb2:
+; CHECK-NEXT: [[TMP4:%.*]] = getelementptr [[STRUCT_FOO:%.*]], ptr [[TMP]], i64 0, i32 1
; CHECK-NEXT: br i1 undef, label [[BB2]], label [[BB7:%.*]]
; CHECK: bb7:
; CHECK-NEXT: br label [[BB10:%.*]]
diff --git a/llvm/test/Transforms/NewGVN/tbaa.ll b/llvm/test/Transforms/NewGVN/tbaa.ll
index 335e782acc8bcd..85ee6a1293a7e9 100644
--- a/llvm/test/Transforms/NewGVN/tbaa.ll
+++ b/llvm/test/Transforms/NewGVN/tbaa.ll
@@ -95,6 +95,7 @@ define i32 @test7(ptr %p, ptr %q) {
define i32 @test8(ptr %p, ptr %q) {
; CHECK-LABEL: define i32 @test8(
; CHECK-SAME: ptr [[P:%.*]], ptr [[Q:%.*]]) {
+; CHECK-NEXT: [[A:%.*]] = load i32, ptr [[Q]], align 4, !tbaa [[TBAA7:![0-9]+]]
; CHECK-NEXT: store i32 15, ptr [[P]], align 4
; CHECK-NEXT: ret i32 0
;
@@ -111,6 +112,7 @@ define i32 @test8(ptr %p, ptr %q) {
define i32 @test9(ptr %p, ptr %q) {
; CHECK-LABEL: define i32 @test9(
; CHECK-SAME: ptr [[P:%.*]], ptr [[Q:%.*]]) {
+; CHECK-NEXT: [[A:%.*]] = load i32, ptr [[Q]], align 4, !tbaa [[TBAA7]]
; CHECK-NEXT: call void @clobber()
; CHECK-NEXT: ret i32 0
;
@@ -129,7 +131,7 @@ define i32 @test10(ptr %p, ptr %q) {
; and not just the common final access type.
; CHECK-LABEL: define i32 @test10(
; CHECK-SAME: ptr [[P:%.*]], ptr [[Q:%.*]]) {
-; CHECK-NEXT: [[A:%.*]] = call i32 @foo(ptr [[P]]), !tbaa [[TBAA7:![0-9]+]]
+; CHECK-NEXT: [[A:%.*]] = call i32 @foo(ptr [[P]]), !tbaa [[TBAA10:![0-9]+]]
; CHECK-NEXT: [[C:%.*]] = add i32 [[A]], [[A]]
; CHECK-NEXT: ret i32 [[C]]
;
@@ -172,8 +174,11 @@ declare i32 @foo(ptr) readonly
; CHECK: [[TBAA4]] = !{[[META5:![0-9]+]], [[META5]], i64 0}
; CHECK: [[META5]] = !{!"B", [[META2]]}
; CHECK: [[TBAA6]] = !{[[META2]], [[META2]], i64 0}
-; CHECK: [[TBAA7]] = !{[[META8:![0-9]+]], [[META9:![0-9]+]], i64 0}
-; CHECK: [[META8]] = !{!"struct X", [[META9]], i64 0}
-; CHECK: [[META9]] = !{!"int", [[META10:![0-9]+]], i64 0}
-; CHECK: [[META10]] = !{!"char", [[META3]], i64 0}
+; CHECK: [[TBAA7]] = !{[[META8:![0-9]+]], [[META8]], i64 0, i64 1}
+; CHECK: [[META8]] = !{!"node", [[META9:![0-9]+]]}
+; CHECK: [[META9]] = !{!"yet another root"}
+; CHECK: [[TBAA10]] = !{[[META11:![0-9]+]], [[META12:![0-9]+]], i64 0}
+; CHECK: [[META11]] = !{!"struct X", [[META12]], i64 0}
+; CHECK: [[META12]] = !{!"int", [[META13:![0-9]+]], i64 0}
+; CHECK: [[META13]] = !{!"char", [[META3]], i64 0}
;.
>From 1923364859ae6116bae7c1c7b7b8cb1be5ec3d89 Mon Sep 17 00:00:00 2001
From: ManuelJBrito <59119670+ManuelJBrito at users.noreply.github.com>
Date: Thu, 26 Dec 2024 20:29:35 +0000
Subject: [PATCH 2/2] Update pr31682.ll
---
llvm/test/Transforms/NewGVN/pr31682.ll | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/test/Transforms/NewGVN/pr31682.ll b/llvm/test/Transforms/NewGVN/pr31682.ll
index 69c64251b96e1c..e7ffefad655898 100644
--- a/llvm/test/Transforms/NewGVN/pr31682.ll
+++ b/llvm/test/Transforms/NewGVN/pr31682.ll
@@ -13,7 +13,7 @@ define void @bar(i1 %arg) {
; CHECK-NEXT: br label [[BB2:%.*]]
; CHECK: bb2:
; CHECK-NEXT: [[TMP4:%.*]] = getelementptr [[STRUCT_FOO:%.*]], ptr [[TMP]], i64 0, i32 1
-; CHECK-NEXT: br i1 undef, label [[BB2]], label [[BB7:%.*]]
+; CHECK-NEXT: br i1 %arg, label [[BB2]], label [[BB7:%.*]]
; CHECK: bb7:
; CHECK-NEXT: br label [[BB10:%.*]]
; CHECK: bb10:
More information about the llvm-commits
mailing list