[DSE] Delay deleting non-memory-defs until end of DSE. #83411

fhahn · 2024-02-29T11:27:42Z

DSE uses BatchAA, which caches queries using pairs of MemoryLocations. At the moment, DSE may remove instructions that are used as pointers in cached MemoryLocations. If a new instruction used by a new MemoryLoation and this instruction gets allocated at the same address as a previosuly cached and then removed instruction, we may access an incorrect entry in the cache.

To avoid this delay removing all instructions except MemoryDefs until the end of DSE. This should avoid removing any values used in BatchAA's cache.

Test case by @vporpo from
#83181.
(Test not precommitted because the results are non-determinstic - memset only sometimes gets removed)

@vporpo

DSE uses BatchAA, which caches queries using pairs of MemoryLocations. At the moment, DSE may remove instructions that are used as pointers in cached MemoryLocations. If a new instruction used by a new MemoryLoation and this instruction gets allocated at the same address as a previosuly cached and then removed instruction, we may access an incorrect entry in the cache. To avoid this delay removing all instructions except MemoryDefs until the end of DSE. This should avoid removing any values used in BatchAA's cache. Test case by @vporpo from llvm#83181. (Test not precommitted because the results are non-determinstic - memset only sometimes gets removed)

fhahn · 2024-02-29T11:30:09Z

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp

+      // instructions to be allocated at the same address, yielding stale cache
+      // entries.
+      if (IsMemDef)
+        DeadInst->eraseFromParent();


Not sure if there could be cases where a MemoryDef also loads a value that is then used as pointer of MemoryLocation

Is it safer to just delay the deletion for them too?

+1 to this question, I'm not clear if this is possible. MemoryDefs could be volatile loads, but this is not the case here since the pass is only looking at store/writing instructions. A memcpy does also load, so a memcpy to a location that is later overwritten, could be deleted here, while copying from a location that's already in the cache.

At this time I'm inclined to say this cannot happen. So let's have this fix in for the existing issue.

Deleting a memcpy wouldn't be a problem, as long as its pointer operands aren't deleted.

But we might remove something like a malloc call, which is a memory def, and the result of the malloc could be used as address and be in the cache. Updated the code to remove memory-defs that do not produce a value (i.e. have type void)

Is it possible to add more tests to cover the void/non-void type cases mentioned?

Added a couple of extra tests based on the original test.

nikic

There's a build failure, but conceptually this fix looks right to me.

fhahn · 2024-02-29T12:50:12Z

Build failure should be fixed, thanks!

llvmbot · 2024-02-29T12:50:20Z

@llvm/pr-subscribers-llvm-transforms

Author: Florian Hahn (fhahn)

Changes

DSE uses BatchAA, which caches queries using pairs of MemoryLocations. At the moment, DSE may remove instructions that are used as pointers in cached MemoryLocations. If a new instruction used by a new MemoryLoation and this instruction gets allocated at the same address as a previosuly cached and then removed instruction, we may access an incorrect entry in the cache.

To avoid this delay removing all instructions except MemoryDefs until the end of DSE. This should avoid removing any values used in BatchAA's cache.

Test case by @vporpo from
#83181.
(Test not precommitted because the results are non-determinstic - memset only sometimes gets removed)

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

2 Files Affected:

(modified) llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp (+27-5)
(added) llvm/test/Transforms/DeadStoreElimination/batchaa-caching-new-pointers.ll (+42)

diff --git a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
index d30c68a2f08712..214c10c52be4a8 100644
--- a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
@@ -857,6 +857,9 @@ struct DSEState {
   // no longer be captured.
   bool ShouldIterateEndOfFunctionDSE;
 
+  /// Dead instructions to be removed at the end of DSE.
+  SmallVector<Instruction *> ToRemove;
+
   // Class contains self-reference, make sure it's not copied/moved.
   DSEState(const DSEState &) = delete;
   DSEState &operator=(const DSEState &) = delete;
@@ -1692,7 +1695,8 @@ struct DSEState {
     return {MaybeDeadAccess};
   }
 
-  // Delete dead memory defs
+  /// Delete dead memory defs and recursively add their operands to ToRemove if
+  /// they became dead.
   void deleteDeadInstruction(Instruction *SI) {
     MemorySSAUpdater Updater(&MSSA);
     SmallVector<Instruction *, 32> NowDeadInsts;
@@ -1708,8 +1712,11 @@ struct DSEState {
       salvageKnowledge(DeadInst);
 
       // Remove the Instruction from MSSA.
-      if (MemoryAccess *MA = MSSA.getMemoryAccess(DeadInst)) {
-        if (MemoryDef *MD = dyn_cast<MemoryDef>(MA)) {
+      MemoryAccess *MA = MSSA.getMemoryAccess(DeadInst);
+      bool IsMemDef = MA && isa<MemoryDef>(MA);
+      if (MA) {
+        if (IsMemDef) {
+          auto *MD = cast<MemoryDef>(MA);
           SkipStores.insert(MD);
           if (auto *SI = dyn_cast<StoreInst>(MD->getMemoryInst())) {
             if (SI->getValueOperand()->getType()->isPointerTy()) {
@@ -1730,13 +1737,21 @@ struct DSEState {
       // Remove its operands
       for (Use &O : DeadInst->operands())
         if (Instruction *OpI = dyn_cast<Instruction>(O)) {
-          O = nullptr;
+          O.set(PoisonValue::get(O->getType()));
           if (isInstructionTriviallyDead(OpI, &TLI))
             NowDeadInsts.push_back(OpI);
         }
 
       EI.removeInstruction(DeadInst);
-      DeadInst->eraseFromParent();
+      // Remove memory defs directly, but only queue other dead instructions for
+      // later removal. They may have been used as memory locations that have
+      // been cached by BatchAA. Removing them here may lead to newly created
+      // instructions to be allocated at the same address, yielding stale cache
+      // entries.
+      if (IsMemDef)
+        DeadInst->eraseFromParent();
+      else
+        ToRemove.push_back(DeadInst);
     }
   }
 
@@ -2287,6 +2302,13 @@ static bool eliminateDeadStores(Function &F, AliasAnalysis &AA, MemorySSA &MSSA,
 
   MadeChange |= State.eliminateRedundantStoresOfExistingValues();
   MadeChange |= State.eliminateDeadWritesAtEndOfFunction();
+
+  while (!State.ToRemove.empty()) {
+    Instruction *DeadInst = State.ToRemove.pop_back_val();
+    assert(!MSSA.getMemoryAccess(DeadInst));
+    DeadInst->eraseFromParent();
+  }
+
   return MadeChange;
 }
 } // end anonymous namespace
diff --git a/llvm/test/Transforms/DeadStoreElimination/batchaa-caching-new-pointers.ll b/llvm/test/Transforms/DeadStoreElimination/batchaa-caching-new-pointers.ll
new file mode 100644
index 00000000000000..0e6eb8e7ef8d5a
--- /dev/null
+++ b/llvm/test/Transforms/DeadStoreElimination/batchaa-caching-new-pointers.ll
@@ -0,0 +1,42 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt -S -passes=dse < %s | FileCheck %s
+;
+; DSE kills `store i32 44, ptr %struct.byte.4, align 4` but should not kill
+; `call void @llvm.memset.p0.i64(...)`  because it has a clobber read:
+; `%ret = load ptr, ptr %struct.byte.8`
+
+
+%struct.type = type { ptr, ptr }
+
+define ptr @foo(ptr noundef %ptr) {
+; CHECK-LABEL: define ptr @foo(
+; CHECK-SAME: ptr noundef [[PTR:%.*]]) {
+; CHECK-NEXT:    [[STRUCT_ALLOCA:%.*]] = alloca [[STRUCT_TYPE:%.*]], align 8
+; CHECK-NEXT:    call void @llvm.lifetime.start.p0(i64 56, ptr nonnull [[STRUCT_ALLOCA]]) #[[ATTR2:[0-9]+]]
+; CHECK-NEXT:    [[STRUCT_BYTE_8:%.*]] = getelementptr inbounds i8, ptr [[STRUCT_ALLOCA]], i64 8
+; CHECK-NEXT:    [[TMP1:%.*]] = getelementptr inbounds i8, ptr [[STRUCT_BYTE_8]], i64 4
+; CHECK-NEXT:    call void @llvm.memset.p0.i64(ptr noundef nonnull align 4 [[TMP1]], i8 42, i64 4, i1 false)
+; CHECK-NEXT:    store i32 43, ptr [[STRUCT_BYTE_8]], align 4
+; CHECK-NEXT:    [[RET:%.*]] = load ptr, ptr [[STRUCT_BYTE_8]], align 8
+; CHECK-NEXT:    call void @llvm.lifetime.end.p0(i64 56, ptr nonnull [[STRUCT_ALLOCA]]) #[[ATTR2]]
+; CHECK-NEXT:    ret ptr [[RET]]
+;
+  %struct.alloca = alloca %struct.type, align 8
+  call void @llvm.lifetime.start.p0(i64 56, ptr nonnull %struct.alloca) nounwind
+  %struct.byte.8 = getelementptr inbounds i8, ptr %struct.alloca, i64 8
+  ; Set %struct.alloca[8, 16) to 42.
+  call void @llvm.memset.p0.i64(ptr noundef nonnull align 4 %struct.byte.8, i8 42, i64 8, i1 false)
+  ; Set %struct.alloca[8, 12) to 43.
+  store i32 43, ptr %struct.byte.8, align 4
+  ; Set %struct.alloca[4, 8) to 44.
+  %struct.byte.4 = getelementptr inbounds i8, ptr %struct.alloca, i64 4
+  store i32 44, ptr %struct.byte.4, align 4
+  ; Return %struct.alloca[8, 16).
+  %ret = load ptr, ptr %struct.byte.8
+  call void @llvm.lifetime.end.p0(i64 56, ptr nonnull %struct.alloca) nounwind
+  ret ptr %ret
+}
+
+declare void @llvm.memset.p0.i64(ptr nocapture writeonly, i8, i64, i1 immarg)
+declare void @llvm.lifetime.end.p0(i64 immarg, ptr nocapture)
+declare void @llvm.lifetime.start.p0(i64 immarg, ptr nocapture)

vporpo · 2024-02-29T16:32:10Z

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp

+
+  while (!State.ToRemove.empty()) {
+    Instruction *DeadInst = State.ToRemove.pop_back_val();
+    assert(!MSSA.getMemoryAccess(DeadInst));


Nit: Perhaps an assertion comment along the lines of "Dead MemDefs are erased right away"?

I went ahead and dropped the assert, as it was incorrect and never triggered, due to memory accesses always being removed from MemorySSA.

Also, in the latest version, we may remove memory-defs here, if they produce values.

alinas · 2024-02-29T19:41:53Z

An alternative to delaying removes is instantiating a new BatchAA when you need to call createInBounds, effectively dropping the existing cache.

alinas · 2024-03-01T16:18:52Z

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp

+      // instructions to be allocated at the same address, yielding stale cache
+      // entries.
+      if (IsMemDef)
+        DeadInst->eraseFromParent();


At this time I'm inclined to say this cannot happen. So let's have this fix in for the existing issue.

@vporpo

DSE uses BatchAA, which caches queries using pairs of MemoryLocations. At the moment, DSE may remove instructions that are used as pointers in cached MemoryLocations. If a new instruction used by a new MemoryLoation and this instruction gets allocated at the same address as a previosuly cached and then removed instruction, we may access an incorrect entry in the cache. To avoid this delay removing all instructions except MemoryDefs until the end of DSE. This should avoid removing any values used in BatchAA's cache. Test case by @vporpo from llvm#83181. (Test not precommitted because the results are non-determinstic - memset only sometimes gets removed) PR: llvm#83411

[DSE] Delay deleting non-memory-defs until end of DSE. (llvm#83411)

nikic · 2024-03-06T20:12:21Z

/cherry-pick 10f5e98

@vporpo

DSE uses BatchAA, which caches queries using pairs of MemoryLocations. At the moment, DSE may remove instructions that are used as pointers in cached MemoryLocations. If a new instruction used by a new MemoryLoation and this instruction gets allocated at the same address as a previosuly cached and then removed instruction, we may access an incorrect entry in the cache. To avoid this delay removing all instructions except MemoryDefs until the end of DSE. This should avoid removing any values used in BatchAA's cache. Test case by @vporpo from llvm#83181. (Test not precommitted because the results are non-determinstic - memset only sometimes gets removed) PR: llvm#83411 (cherry picked from commit 10f5e98)

llvmbot · 2024-03-06T20:17:29Z

/pull-request #84227

@vporpo

DSE uses BatchAA, which caches queries using pairs of MemoryLocations. At the moment, DSE may remove instructions that are used as pointers in cached MemoryLocations. If a new instruction used by a new MemoryLoation and this instruction gets allocated at the same address as a previosuly cached and then removed instruction, we may access an incorrect entry in the cache. To avoid this delay removing all instructions except MemoryDefs until the end of DSE. This should avoid removing any values used in BatchAA's cache. Test case by @vporpo from llvm#83181. (Test not precommitted because the results are non-determinstic - memset only sometimes gets removed) PR: llvm#83411 (cherry picked from commit 10f5e98)

fhahn requested review from nikic, alinas, vporpo, aeubanks and david-xl February 29, 2024 11:27

fhahn mentioned this pull request Feb 29, 2024

[DSE] Fixes bug caused by skipped read clobbers #83181

Closed

fhahn commented Feb 29, 2024

View reviewed changes

nikic reviewed Feb 29, 2024

View reviewed changes

fhahn added 2 commits February 29, 2024 12:41

Merge remote-tracking branch 'origin/main' into perf/dse-delay-delete

952b51b

!fixup fix build failure.

33f9631

llvmbot added the llvm:transforms label Feb 29, 2024

vporpo reviewed Feb 29, 2024

View reviewed changes

alinas approved these changes Mar 1, 2024

View reviewed changes

fhahn added 4 commits March 1, 2024 16:37

Merge remote-tracking branch 'origin/main' into perf/dse-delay-delete

1d036f4

!fixup remove assert, directly delete MemDefs with void ty.

be3aaa1

Merge remote-tracking branch 'origin/main' into perf/dse-delay-delete

8fc503c

!fixup add extra tests.

c839e00

fhahn merged commit 10f5e98 into llvm:main Mar 2, 2024
3 of 4 checks passed

fhahn deleted the perf/dse-delay-delete branch March 2, 2024 12:34

fhahn mentioned this pull request Mar 4, 2024

[DSE] Delay deleting non-memory-defs until end of DSE. (#83411) swiftlang/llvm-project#8323

Merged

fhahn added a commit to swiftlang/llvm-project that referenced this pull request Mar 5, 2024

Merge pull request #8323 from fhahn/dse-fix

46f35c9

[DSE] Delay deleting non-memory-defs until end of DSE. (llvm#83411)

nikic added this to the LLVM 18.X Release milestone Mar 6, 2024

pointhex mentioned this pull request May 7, 2024

getStyleDiagHandler #91314

Closed

aemerson mentioned this pull request May 9, 2024

release/18.x: [AArc64][GlobalISel] Fix legalizer assert for G_INSERT_VECTOR_ELT - manual merge #91672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DSE] Delay deleting non-memory-defs until end of DSE. #83411

[DSE] Delay deleting non-memory-defs until end of DSE. #83411

fhahn commented Feb 29, 2024

fhahn Feb 29, 2024

david-xl Feb 29, 2024

alinas Feb 29, 2024

alinas Mar 1, 2024

fhahn Mar 1, 2024

david-xl Mar 1, 2024

fhahn Mar 2, 2024

nikic left a comment

fhahn commented Feb 29, 2024

llvmbot commented Feb 29, 2024

vporpo Feb 29, 2024

fhahn Mar 1, 2024

alinas commented Feb 29, 2024

alinas Mar 1, 2024

nikic commented Mar 6, 2024

llvmbot commented Mar 6, 2024

[DSE] Delay deleting non-memory-defs until end of DSE. #83411

[DSE] Delay deleting non-memory-defs until end of DSE. #83411

Conversation

fhahn commented Feb 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment

fhahn commented Feb 29, 2024

llvmbot commented Feb 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alinas commented Feb 29, 2024

Choose a reason for hiding this comment

nikic commented Mar 6, 2024

llvmbot commented Mar 6, 2024