[llvm] [DSE] Fix non-determinism due to address reuse (PR #84943)
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 11 02:15:51 PDT 2024
https://github.com/nikic updated https://github.com/llvm/llvm-project/pull/84943
>From 665649d318f588b303b1a273b60336240d67787b Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Tue, 12 Mar 2024 17:25:56 +0100
Subject: [PATCH 1/2] [DSE] Fix non-determinism due to address reuse
The malloc->calloc fold creates a new MemoryAccess, which may end
of at the same address as a previously deleted access inside
SkipStores.
To the most part, this is not a problem, because SkipStores is
normally only used together with MemDefs. Neither the old malloc
access nor the new calloc access will be part of MemDefs, so
there is no problem here.
However, SkipStores is also used in one more place: In the main
DSE loop, ToCheck entries are checked against it. Fix this by
not using SkipStores here, and instead using a separate set to
track deletions inside this loop. This way it is not affected
by the calloc optimization that happens outside it.
This is all pretty ugly, but I haven't found another good way to
fix it. Suggestions welcome.
No test case as I don't have a reliable DSE-only test-case for
this.
Fixes https://github.com/llvm/llvm-project/issues/84458.
---
.../Transforms/Scalar/DeadStoreElimination.cpp | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
index bfc8bd5970bf27f..fb0d7f5b7eb261f 100644
--- a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
@@ -1699,7 +1699,9 @@ struct DSEState {
/// Delete dead memory defs and recursively add their operands to ToRemove if
/// they became dead.
- void deleteDeadInstruction(Instruction *SI) {
+ void
+ deleteDeadInstruction(Instruction *SI,
+ SmallPtrSetImpl<MemoryAccess *> *Deleted = nullptr) {
MemorySSAUpdater Updater(&MSSA);
SmallVector<Instruction *, 32> NowDeadInsts;
NowDeadInsts.push_back(SI);
@@ -1720,6 +1722,8 @@ struct DSEState {
if (IsMemDef) {
auto *MD = cast<MemoryDef>(MA);
SkipStores.insert(MD);
+ if (Deleted)
+ Deleted->insert(MD);
if (auto *SI = dyn_cast<StoreInst>(MD->getMemoryInst())) {
if (SI->getValueOperand()->getType()->isPointerTy()) {
const Value *UO = getUnderlyingObject(SI->getValueOperand());
@@ -2168,6 +2172,7 @@ static bool eliminateDeadStores(Function &F, AliasAnalysis &AA, MemorySSA &MSSA,
unsigned PartialLimit = MemorySSAPartialStoreLimit;
// Worklist of MemoryAccesses that may be killed by KillingDef.
SmallSetVector<MemoryAccess *, 8> ToCheck;
+ SmallPtrSet<MemoryAccess *, 8> Deleted;
ToCheck.insert(KillingDef->getDefiningAccess());
bool Shortend = false;
@@ -2175,7 +2180,7 @@ static bool eliminateDeadStores(Function &F, AliasAnalysis &AA, MemorySSA &MSSA,
// Check if MemoryAccesses in the worklist are killed by KillingDef.
for (unsigned I = 0; I < ToCheck.size(); I++) {
MemoryAccess *Current = ToCheck[I];
- if (State.SkipStores.count(Current))
+ if (Deleted.contains(Current))
continue;
std::optional<MemoryAccess *> MaybeDeadAccess = State.getDomMemoryDef(
@@ -2222,7 +2227,7 @@ static bool eliminateDeadStores(Function &F, AliasAnalysis &AA, MemorySSA &MSSA,
continue;
LLVM_DEBUG(dbgs() << "DSE: Remove Dead Store:\n DEAD: " << *DeadI
<< "\n KILLER: " << *KillingI << '\n');
- State.deleteDeadInstruction(DeadI);
+ State.deleteDeadInstruction(DeadI, &Deleted);
++NumFastStores;
MadeChange = true;
} else {
@@ -2259,7 +2264,7 @@ static bool eliminateDeadStores(Function &F, AliasAnalysis &AA, MemorySSA &MSSA,
Shortend = true;
// Remove killing store and remove any outstanding overlap
// intervals for the updated store.
- State.deleteDeadInstruction(KillingSI);
+ State.deleteDeadInstruction(KillingSI, &Deleted);
auto I = State.IOLs.find(DeadSI->getParent());
if (I != State.IOLs.end())
I->second.erase(DeadSI);
@@ -2271,7 +2276,7 @@ static bool eliminateDeadStores(Function &F, AliasAnalysis &AA, MemorySSA &MSSA,
if (OR == OW_Complete) {
LLVM_DEBUG(dbgs() << "DSE: Remove Dead Store:\n DEAD: " << *DeadI
<< "\n KILLER: " << *KillingI << '\n');
- State.deleteDeadInstruction(DeadI);
+ State.deleteDeadInstruction(DeadI, &Deleted);
++NumFastStores;
MadeChange = true;
}
>From 23e7f5f2da8becf5e77848e4d4854b51674ad8b5 Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Thu, 11 Apr 2024 18:15:30 +0900
Subject: [PATCH 2/2] Add comment and assertion
---
llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
index fb0d7f5b7eb261f..ed4212d29cef707 100644
--- a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
@@ -2172,7 +2172,11 @@ static bool eliminateDeadStores(Function &F, AliasAnalysis &AA, MemorySSA &MSSA,
unsigned PartialLimit = MemorySSAPartialStoreLimit;
// Worklist of MemoryAccesses that may be killed by KillingDef.
SmallSetVector<MemoryAccess *, 8> ToCheck;
+ // Track MemoryAccesses that have been deleted in the loop below, so we can
+ // skip them. Don't use SkipStores for this, which may contain reused
+ // MemoryAccess addresses.
SmallPtrSet<MemoryAccess *, 8> Deleted;
+ [[maybe_unused]] unsigned OrigNumSkipStores = State.SkipStores.size();
ToCheck.insert(KillingDef->getDefiningAccess());
bool Shortend = false;
@@ -2283,6 +2287,9 @@ static bool eliminateDeadStores(Function &F, AliasAnalysis &AA, MemorySSA &MSSA,
}
}
+ assert(State.SkipStores.size() - OrigNumSkipStores == Deleted.size() &&
+ "SkipStores and Deleted out of sync?");
+
// Check if the store is a no-op.
if (!Shortend && State.storeIsNoop(KillingDef, KillingUndObj)) {
LLVM_DEBUG(dbgs() << "DSE: Remove No-Op Store:\n DEAD: " << *KillingI
More information about the llvm-commits
mailing list