[DSE] Fix non-determinism due to address reuse

[nikic]

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 #84458.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Nikita Popov (nikic)

Changes

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 #84458.

---

Full diff: https://github.com/llvm/llvm-project/pull/84943.diff

1 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp (+10-5)

diff --git a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
index 392e6ad5a66bb9..865999cb7ca30b 100644
--- a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
@@ -1698,7 +1698,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);
@@ -1719,6 +1721,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());
@@ -2167,6 +2171,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;
@@ -2174,7 +2179,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(
@@ -2221,7 +2226,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 {
@@ -2258,7 +2263,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);
@@ -2270,7 +2275,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;
         }

[nikic]

ping

[nikic]

ping

[fhahn]

Thanks for the patch, I can't really think of a better solution at the moment.

[fhahn]

Could you add a comment here explaining why this extra set is needed here?

[fhahn]

Would it be possible to check if all uses in the loop pass Deleted, e.g. add an assert that checks if we add as many elements to SkipStore as we do to Deleted after the loop?

[nikic]

I've added an assertion -- does that match what you had in mind?

[fhahn]

Thanks!

[fhahn]

LGTM, thanks!