[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.

This patch additionally fixes some function names
to conform with the LLVM style guide.

Fixes #64680.

Change-Id: I972dc76758ced16cefcb08474b756ec89e0c047f

Author: gandhi56

llvm/lib/Transforms/IPO/GlobalOpt.cpp

@@ -160,12 +160,12 @@ static bool isLeakCheckerRoot(GlobalVariable *GV) {
 /// Given a value that is stored to a global but never read, determine whether
 /// it's safe to remove the store and the chain of computation that feeds the
 /// store.
-static bool IsSafeComputationToRemove(
+static bool isSafeComputationToRemove(
     Value *V, function_ref<TargetLibraryInfo &(Function &)> GetTLI) {
   do {
     if (isa<Constant>(V))
       return true;
-    if (!V->hasOneUse())
+    if (V->hasNUsesOrMore(1))
       return false;
     if (isa<LoadInst>(V) || isa<InvokeInst>(V) || isa<Argument>(V) ||
         isa<GlobalValue>(V))
@@ -187,11 +187,21 @@ static bool IsSafeComputationToRemove(
   } while (true);
 }
 
-/// This GV is a pointer root.  Loop over all users of the global and clean up
-/// any that obviously don't assign the global a value that isn't dynamically
-/// allocated.
+/**
+ * Cleans up pointer root users of a global variable.
+ *
+ * This function iterates over all users of the global variable and collects
+ * all stores and memtransfer instructions. It then erases all writes and
+ * removes computation chains if they are safe to remove. Finally, it removes
+ * dead constant users of the global variable.
+ *
+ * @param GV The global variable to clean up.
+ * @param GetTLI A function reference to obtain the TargetLibraryInfo for a
+ * given function.
+ * @return True if any changes were made, false otherwise.
+ */
 static bool
-CleanupPointerRootUsers(GlobalVariable *GV,
+cleanupPointerRootUsers(GlobalVariable *GV,
                         function_ref<TargetLibraryInfo &(Function &)> GetTLI) {
   // A brief explanation of leak checkers.  The goal is to find bugs where
   // pointers are forgotten, causing an accumulating growth in memory
@@ -204,60 +214,38 @@ 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;
-
+  // Iterate over all users of the global and collect all
+  // stores, memtransfer and memset instructions.
+  SmallVector<std::pair<Instruction *, Value *>> 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));
+    if (auto *SI = dyn_cast<StoreInst>(U)) {
+      Writes.push_back({SI, SI->getValueOperand()});
+    } else if (auto *CE = dyn_cast<ConstantExpr>(U)) {
+      if (isa<GEPOperator>(CE)) {
+        append_range(Worklist, CE->users());
       }
-    } 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 (auto *MTI = dyn_cast<MemTransferInst>(U)) {
+      if (MTI->getRawDest() == GV) {
+        Writes.push_back({MTI, MTI->getSource()});
       }
-    } 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 (auto *MSI = dyn_cast<MemSetInst>(U)) {
+      if (MSI->getRawDest() == GV) {
+        Writes.push_back({MSI, MSI->getValue()});
       }
-    } else if (ConstantExpr *CE = dyn_cast<ConstantExpr>(U)) {
-      if (isa<GEPOperator>(CE))
-        append_range(Worklist, CE->users());
     }
   }
 
-  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);
-      I->eraseFromParent();
+  // Finally, erase all writes and remove computation chains if they are safe
+  // to remove.
+  for (auto [WriteInst, V] : Writes) {
+    if (isa<Constant>(V) || isa<Instruction>(V))
+      WriteInst->eraseFromParent();
+
+    if (auto *Inst = dyn_cast<Instruction>(V)) {
+      if (isSafeComputationToRemove(V, GetTLI))
+        RecursivelyDeleteTriviallyDeadInstructions(V);
       Changed = true;
     }
   }
@@ -872,7 +860,7 @@ static bool OptimizeAwayTrappingUsesOfLoads(
   // nor is the global.
   if (AllNonStoreUsesGone) {
     if (isLeakCheckerRoot(GV)) {
-      Changed |= CleanupPointerRootUsers(GV, GetTLI);
+      Changed |= cleanupPointerRootUsers(GV, GetTLI);
     } else {
       Changed = true;
       CleanupConstantGlobalUsers(GV, DL);
@@ -1494,7 +1482,7 @@ processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS,
 
     if (isLeakCheckerRoot(GV)) {
       // Delete any constant stores to the global.
-      Changed = CleanupPointerRootUsers(GV, GetTLI);
+      Changed = cleanupPointerRootUsers(GV, GetTLI);
     } else {
       // Delete any stores we can find to the global.  We may not be able to
       // make it completely dead though.

llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users-gep-constexpr.ll

@@ -99,7 +99,6 @@ 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

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

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) }
-;.