Skip to content

Conversation

@AlexMaclean
Copy link
Member

No description provided.

@llvmbot
Copy link
Member

llvmbot commented Jan 11, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Alex MacLean (AlexMaclean)

Changes

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

2 Files Affected:

  • (modified) llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp (+3-1)
  • (modified) llvm/test/Transforms/DeadStoreElimination/noop-stores.ll (+11)
diff --git a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
index cae5b9c41a37f1..908b9110bd595d 100644
--- a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
@@ -2116,7 +2116,9 @@ struct DSEState {
       return true;
 
     if (auto *LoadI = dyn_cast<LoadInst>(Store->getOperand(0))) {
-      if (LoadI->getPointerOperand() == Store->getOperand(1)) {
+      if (LoadI->getPointerOperand() == Store->getOperand(1) ||
+          AA.isMustAlias(MemoryLocation::get(LoadI),
+                         MemoryLocation::get(Store))) {
         // Get the defining access for the load.
         auto *LoadAccess = MSSA.getMemoryAccess(LoadI)->getDefiningAccess();
         // Fast path: the defining accesses are the same.
diff --git a/llvm/test/Transforms/DeadStoreElimination/noop-stores.ll b/llvm/test/Transforms/DeadStoreElimination/noop-stores.ll
index 9fc20d76da5eb4..5bedc1ebe8281e 100644
--- a/llvm/test/Transforms/DeadStoreElimination/noop-stores.ll
+++ b/llvm/test/Transforms/DeadStoreElimination/noop-stores.ll
@@ -1152,3 +1152,14 @@ if.else:
 end:
   ret void
 }
+
+define i32 @test_use_alias_analysis(ptr %Q) {
+; CHECK-LABEL: @test_use_alias_analysis(
+; CHECK-NEXT:    [[A:%.*]] = load i32, ptr [[Q:%.*]], align 4
+; CHECK-NEXT:    ret i32 [[A]]
+;
+  %a = load i32, ptr %Q
+  %Q2 = addrspacecast ptr %Q to ptr addrspace(5)
+  store i32 %a, ptr addrspace(5) %Q2
+  ret i32 %a
+}

@nikic nikic requested review from fhahn and nikic January 11, 2025 21:08
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@fhahn fhahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compile-time: http://llvm-compile-time-tracker.com/compare.php?from=bfe93aedcc7d393c2697e66d6569baffb701ba6f&to=d41dfc385f0a808507612043cf76924d721aa325&stat=instructions:u

Hmm seems quite high for no changes. @AlexMaclean do you have any data on how many additional stores can be removed with this change on larger workloads?

if (auto *LoadI = dyn_cast<LoadInst>(Store->getOperand(0))) {
if (LoadI->getPointerOperand() == Store->getOperand(1)) {
if (LoadI->getPointerOperand() == Store->getOperand(1) ||
AA.isMustAlias(MemoryLocation::get(LoadI),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming my understanding of the problem: checking that the address spaces are not equal is insufficient because address spaces may not be disjoint, correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants