[llvm] [GlobalOpt] Handle operators separately when removing GV users (PR #84694)

Anshil Gandhi via llvm-commits llvm-commits at lists.llvm.org
Thu Apr 25 10:42:01 PDT 2024


https://github.com/gandhi56 updated https://github.com/llvm/llvm-project/pull/84694

>From 09c23889c8310b7fdc3b1db7320754d354f48296 Mon Sep 17 00:00:00 2001
From: Anshil Gandhi <gandhi21299 at gmail.com>
Date: Tue, 2 Apr 2024 20:02:21 -0600
Subject: [PATCH 1/2] [GlobalOpt] Handle operators separately when removing GV
 users

This patch fixes the functionality of CleanupPointerRootUsers
by separately handling cases where the global variable is
wrapped within an operator.

Fixes #64680.

Change-Id: I205922e4203fb032beaf2c290c0374758ea9176c
---
 llvm/lib/Transforms/IPO/GlobalOpt.cpp         | 85 +++++++++----------
 ...leanup-pointer-root-users-gep-constexpr.ll |  2 -
 .../Transforms/GlobalOpt/dead-store-status.ll |  2 -
 llvm/test/Transforms/GlobalOpt/pr54572.ll     |  8 --
 4 files changed, 40 insertions(+), 57 deletions(-)

diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
index da714c9a75701b..fcaf7e8c9ab2f7 100644
--- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -201,61 +201,56 @@ CleanupPointerRootUsers(GlobalVariable *GV,
 
   bool Changed = false;
 
-  // If Dead[n].first is the only use of a malloc result, we can delete its
-  // chain of computation and the store to the global in Dead[n].second.
-  SmallVector<std::pair<Instruction *, Instruction *>, 32> Dead;
-
+  // Collect all stores to GV.
+  SmallVector<Instruction *, 8> Writes;
   SmallVector<User *> Worklist(GV->users());
-  // Constants can't be pointers to dynamically allocated memory.
   while (!Worklist.empty()) {
     User *U = Worklist.pop_back_val();
-    if (StoreInst *SI = dyn_cast<StoreInst>(U)) {
-      Value *V = SI->getValueOperand();
-      if (isa<Constant>(V)) {
-        Changed = true;
-        SI->eraseFromParent();
-      } else if (Instruction *I = dyn_cast<Instruction>(V)) {
-        if (I->hasOneUse())
-          Dead.push_back(std::make_pair(I, SI));
-      }
-    } else if (MemSetInst *MSI = dyn_cast<MemSetInst>(U)) {
-      if (isa<Constant>(MSI->getValue())) {
-        Changed = true;
-        MSI->eraseFromParent();
-      } else if (Instruction *I = dyn_cast<Instruction>(MSI->getValue())) {
-        if (I->hasOneUse())
-          Dead.push_back(std::make_pair(I, MSI));
-      }
-    } else if (MemTransferInst *MTI = dyn_cast<MemTransferInst>(U)) {
-      GlobalVariable *MemSrc = dyn_cast<GlobalVariable>(MTI->getSource());
-      if (MemSrc && MemSrc->isConstant()) {
-        Changed = true;
-        MTI->eraseFromParent();
-      } else if (Instruction *I = dyn_cast<Instruction>(MTI->getSource())) {
-        if (I->hasOneUse())
-          Dead.push_back(std::make_pair(I, MTI));
-      }
-    } else if (ConstantExpr *CE = dyn_cast<ConstantExpr>(U)) {
+    if (auto *SI = dyn_cast<StoreInst>(U)) {
+      Writes.push_back(SI);
+    } else if (auto *CE = dyn_cast<ConstantExpr>(U)) {
       if (isa<GEPOperator>(CE))
         append_range(Worklist, CE->users());
+    } else if (auto *MI = dyn_cast<MemIntrinsic>(U)) {
+      if (MI->getRawDest() == GV)
+        Writes.push_back(MI);
     }
   }
 
-  for (int i = 0, e = Dead.size(); i != e; ++i) {
-    if (IsSafeComputationToRemove(Dead[i].first, GetTLI)) {
-      Dead[i].second->eraseFromParent();
-      Instruction *I = Dead[i].first;
-      do {
-        if (isAllocationFn(I, GetTLI))
-          break;
-        Instruction *J = dyn_cast<Instruction>(I->getOperand(0));
-        if (!J)
-          break;
-        I->eraseFromParent();
-        I = J;
-      } while (true);
+  auto RemoveComputationChain = [&GetTLI](Instruction *I) {
+    do {
+      if (isAllocationFn(I, GetTLI))
+        break;
+      Instruction *J = dyn_cast<Instruction>(I->getOperand(0));
+      if (!J)
+        break;
       I->eraseFromParent();
+      I = J;
+    } while (true);
+    I->eraseFromParent();
+  };
+
+  // Remove the store if it's value operand is a constant or a computation
+  // chain. If the value operand is a computation chain, determine if it is safe
+  // to remove it before removing the store. Remove both the store and the
+  // computation chain if it is safe to do so.
+  for (auto *Write : Writes) {
+    Value *V = Write;
+    if (auto *SI = dyn_cast<StoreInst>(V))
+      V = SI->getValueOperand();
+
+    if (isa<Constant>(V)) {
       Changed = true;
+      Write->eraseFromParent();
+    } else if (isa<Instruction>(V)) {
+      if (IsSafeComputationToRemove(V, GetTLI)) {
+        Changed = true;
+        Write->eraseFromParent();
+        RemoveComputationChain(cast<Instruction>(V));
+      } else {
+        Changed = true;
+        Write->eraseFromParent();
+      }
     }
   }
 
diff --git a/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users-gep-constexpr.ll b/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users-gep-constexpr.ll
index 26728a74d032ca..db0c549ad03f4c 100644
--- a/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users-gep-constexpr.ll
+++ b/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users-gep-constexpr.ll
@@ -99,14 +99,12 @@ define i32 @load_gv_from_op_remove_store(ptr %p) local_unnamed_addr {
 ; CHECK-NEXT:    call void @fn0()
 ; CHECK-NEXT:    br label [[IF_END]]
 ; CHECK:       if.end:
-; CHECK-NEXT:    store ptr [[E]], ptr getelementptr inbounds ([3 x ptr], ptr @b, i64 0, i64 2), align 16
 ; CHECK-NEXT:    [[TMP1:%.*]] = load i32, ptr @a, align 4
 ; CHECK-NEXT:    [[INC:%.*]] = add nsw i32 [[TMP1]], 1
 ; CHECK-NEXT:    store i32 [[INC]], ptr @a, align 4
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp slt i32 [[TMP1]], 2
 ; CHECK-NEXT:    br i1 [[CMP]], label [[FOR_BODY]], label [[FOR_END]]
 ; CHECK:       for.end:
-; CHECK-NEXT:    call void @llvm.memcpy.p0.p0.i64(ptr @b, ptr [[P:%.*]], i64 8, i1 false)
 ; CHECK-NEXT:    ret i32 0
 ;
 entry:
diff --git a/llvm/test/Transforms/GlobalOpt/dead-store-status.ll b/llvm/test/Transforms/GlobalOpt/dead-store-status.ll
index 9a8fbb8d65f0e0..597c08929af902 100644
--- a/llvm/test/Transforms/GlobalOpt/dead-store-status.ll
+++ b/llvm/test/Transforms/GlobalOpt/dead-store-status.ll
@@ -4,8 +4,6 @@
 ; false. This was caught by the pass return status check that is hidden under
 ; EXPENSIVE_CHECKS.
 
-; CHECK: @global = internal unnamed_addr global ptr null, align 1
-
 ; CHECK-LABEL: @foo
 ; CHECK-NEXT: entry:
 ; CHECK-NEXT: ret i16 undef
diff --git a/llvm/test/Transforms/GlobalOpt/pr54572.ll b/llvm/test/Transforms/GlobalOpt/pr54572.ll
index 962c75bd83b415..bdab4b0981d04a 100644
--- a/llvm/test/Transforms/GlobalOpt/pr54572.ll
+++ b/llvm/test/Transforms/GlobalOpt/pr54572.ll
@@ -6,18 +6,10 @@
 
 declare void @llvm.memcpy.p0.p0.i64(ptr noalias nocapture writeonly, ptr noalias nocapture readonly, i64, i1 immarg)
 
-;.
-; CHECK: @[[B:[a-zA-Z0-9_$"\\.-]+]] = internal unnamed_addr global ptr null
-; CHECK: @[[C:[a-zA-Z0-9_$"\\.-]+]] = internal unnamed_addr constant [2 x ptr] zeroinitializer
-;.
 define void @test() {
 ; CHECK-LABEL: @test(
-; CHECK-NEXT:    call void @llvm.memcpy.p0.p0.i64(ptr @b, ptr getelementptr inbounds ([2 x ptr], ptr @c, i64 0, i64 1), i64 8, i1 false)
 ; CHECK-NEXT:    ret void
 ;
   call void @llvm.memcpy.p0.p0.i64(ptr @b, ptr getelementptr inbounds ([2 x ptr], ptr @c, i64 0, i64 1), i64 8, i1 false)
   ret void
 }
-;.
-; CHECK: attributes #[[ATTR0:[0-9]+]] = { nocallback nofree nounwind willreturn memory(argmem: readwrite) }
-;.

>From 639123cbbb182dd36a6c5ce38d56e1cccf83e00d Mon Sep 17 00:00:00 2001
From: Anshil Gandhi <Anshil.Gandhi at amd.com>
Date: Thu, 25 Apr 2024 17:41:36 +0000
Subject: [PATCH 2/2] Remove write operands and write instructions in two
 seperate loops

Change-Id: I80e21ccffbe82dce5aff3a9b1d0d043d36baab12
---
 llvm/lib/Transforms/IPO/GlobalOpt.cpp         | 63 +++++++++----------
 .../GlobalOpt/cleanup-pointer-root-users.ll   |  2 +-
 2 files changed, 30 insertions(+), 35 deletions(-)

diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
index fcaf7e8c9ab2f7..4957c8033f4661 100644
--- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -199,9 +199,20 @@ CleanupPointerRootUsers(GlobalVariable *GV,
   // handle that case, we expect the program may create a singleton and never
   // destroy it.
 
-  bool Changed = false;
+  auto RemoveComputationChain = [&GetTLI](Instruction *I) {
+    do {
+      if (isAllocationFn(I, GetTLI))
+        break;
+      Instruction *J = dyn_cast<Instruction>(I->getOperand(0));
+      if (!J)
+        break;
+      I->eraseFromParent();
+      I = J;
+    } while (true);
+    I->eraseFromParent();
+  };
 
-  // Collect all stores to GV.
+  // Collect all writes to GV.
   SmallVector<Instruction *, 8> Writes;
   SmallVector<User *> Worklist(GV->users());
   while (!Worklist.empty()) {
@@ -217,45 +228,29 @@ CleanupPointerRootUsers(GlobalVariable *GV,
     }
   }
 
-  auto RemoveComputationChain = [&GetTLI](Instruction *I) {
-    do {
-      if (isAllocationFn(I, GetTLI))
-        break;
-      Instruction *J = dyn_cast<Instruction>(I->getOperand(0));
-      if (!J)
-        break;
-      I->eraseFromParent();
-      I = J;
-    } while (true);
-    I->eraseFromParent();
-  };
-
-  // Remove the store if it's value operand is a constant or a computation
-  // chain. If the value operand is a computation chain, determine if it is safe
-  // to remove it before removing the store. Remove both the store and the
-  // computation chain if it is safe to do so.
+  // For each write, try to remove it's operand if it is safe to do so.
   for (auto *Write : Writes) {
-    Value *V = Write;
-    if (auto *SI = dyn_cast<StoreInst>(V))
+    Value *V;
+    if (auto *SI = dyn_cast<StoreInst>(Write)) {
       V = SI->getValueOperand();
+    } else if (auto *MTI = dyn_cast<MemTransferInst>(Write)) {
+      V = MTI->getSource();
+    } else {
+      continue;
+    }
 
-    if (isa<Constant>(V)) {
-      Changed = true;
-      Write->eraseFromParent();
-    } else if (isa<Instruction>(V)) {
-      if (IsSafeComputationToRemove(V, GetTLI)) {
-        Changed = true;
-        Write->eraseFromParent();
-        RemoveComputationChain(cast<Instruction>(V));
-      } else {
-        Changed = true;
-        Write->eraseFromParent();
-      }
+    if (auto *Inst = dyn_cast<Instruction>(V)) {
+      if (IsSafeComputationToRemove(V, GetTLI))
+        RemoveComputationChain(Inst);
     }
   }
 
+  // Finally, remove the writes to GV.
+  for (auto *Write : Writes)
+    Write->eraseFromParent();
+
   GV->removeDeadConstantUsers();
-  return Changed;
+  return !Writes.empty();
 }
 
 /// We just marked GV constant.  Loop over all users of the global, cleaning up
diff --git a/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll b/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll
index aa040297204dca..122e62d7d9cae6 100644
--- a/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll
+++ b/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll
@@ -12,7 +12,7 @@ define void @test1a() {
 
 define void @test1b(ptr %p) {
 ; CHECK-LABEL: @test1b(
-; CHECK-NEXT: store
+; CHECK-NOT: store
 ; CHECK-NEXT: ret void
   store ptr %p, ptr @glbl
   ret void



More information about the llvm-commits mailing list