[GlobalOpt] Improve cleanup of pointer root users

This patch fixes the functionality of CleanupPointerRootUsers
by eliminating code redundancy and refactoring the safety checks
before removing store/memcpy/memmove value operands.

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,15 +160,15 @@ 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) {
+  assert(isa<Instruction>(V) && "Expected an instruction");
   do {
     if (isa<Constant>(V))
       return true;
-    if (!V->hasOneUse())
+    if (V->getNumUses() > 0)
       return false;
-    if (isa<LoadInst>(V) || isa<InvokeInst>(V) || isa<Argument>(V) ||
-        isa<GlobalValue>(V))
+    if (isa<LoadInst, InvokeInst, Argument, GlobalValue>(V))
       return false;
     if (isAllocationFn(V, GetTLI))
       return true;
@@ -191,7 +191,7 @@ static bool IsSafeComputationToRemove(
 /// any that obviously don't assign the global a value that isn't dynamically
 /// allocated.
 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
@@ -203,10 +203,7 @@ CleanupPointerRootUsers(GlobalVariable *GV,
   // destroy it.
 
   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;
+  SmallVector<Instruction *> Dead;
 
   SmallVector<User *> Worklist(GV->users());
   // Constants can't be pointers to dynamically allocated memory.
@@ -218,46 +215,44 @@ CleanupPointerRootUsers(GlobalVariable *GV,
         Changed = true;
         SI->eraseFromParent();
       } else if (Instruction *I = dyn_cast<Instruction>(V)) {
-        if (I->hasOneUse())
-          Dead.push_back(std::make_pair(I, SI));
+        Dead.push_back(I);
+        SI->eraseFromParent();
       }
     } 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));
+        Dead.push_back(I);
+        MSI->eraseFromParent();
       }
     } 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));
+        Dead.push_back(I);
+        MTI->eraseFromParent();
       }
     } 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;
+  for (auto *Inst : Dead) {
+    if (isSafeComputationToRemove(Inst, GetTLI)) {
       do {
-        if (isAllocationFn(I, GetTLI))
+        if (isAllocationFn(Inst, GetTLI))
           break;
-        Instruction *J = dyn_cast<Instruction>(I->getOperand(0));
+        Instruction *J = dyn_cast<Instruction>(Inst->getOperand(0));
         if (!J)
           break;
-        I->eraseFromParent();
-        I = J;
+        Inst->eraseFromParent();
+        Inst = J;
       } while (true);
-      I->eraseFromParent();
+      Inst->eraseFromParent();
       Changed = true;
     }
   }
@@ -269,8 +264,9 @@ CleanupPointerRootUsers(GlobalVariable *GV,
 /// We just marked GV constant.  Loop over all users of the global, cleaning up
 /// the obvious ones.  This is largely just a quick scan over the use list to
 /// clean up the easy and obvious cruft.  This returns true if it made a change.
-static bool CleanupConstantGlobalUsers(GlobalVariable *GV,
-                                       const DataLayout &DL) {
+static bool cleanupConstantGlobalUsers(
+    GlobalVariable *GV, const DataLayout &DL,
+    function_ref<TargetLibraryInfo &(Function &)> GetTLI) {
   Constant *Init = GV->getInitializer();
   SmallVector<User *, 8> WorkList(GV->users());
   SmallPtrSet<User *, 8> Visited;
@@ -872,10 +868,10 @@ 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);
+      cleanupConstantGlobalUsers(GV, DL, GetTLI);
     }
     if (GV->use_empty()) {
       LLVM_DEBUG(dbgs() << "  *** GLOBAL NOW DEAD!\n");
@@ -1494,11 +1490,11 @@ 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.
-      Changed = CleanupConstantGlobalUsers(GV, DL);
+      Changed = cleanupConstantGlobalUsers(GV, DL, GetTLI);
     }
 
     // If the global is dead now, delete it.
@@ -1523,7 +1519,7 @@ processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS,
     }
 
     // Clean up any obviously simplifiable users now.
-    Changed |= CleanupConstantGlobalUsers(GV, DL);
+    Changed |= cleanupConstantGlobalUsers(GV, DL, GetTLI);
 
     // If the global is dead now, just nuke it.
     if (GV->use_empty()) {
@@ -1578,7 +1574,7 @@ processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS,
       }
 
       // Clean up any obviously simplifiable users now.
-      CleanupConstantGlobalUsers(GV, DL);
+      cleanupConstantGlobalUsers(GV, DL, GetTLI);
 
       if (GV->use_empty()) {
         LLVM_DEBUG(dbgs() << "   *** Substituting initializer allowed us to "

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

@@ -1,19 +1,19 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
 ; RUN: opt < %s -passes=globalopt -S | FileCheck %s
 
 ; When removing the store to @global in @foo, the pass would incorrectly return
 ; 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
-
 @global = internal unnamed_addr global ptr null, align 1
 
 ; Function Attrs: nofree noinline norecurse nounwind writeonly
 define i16 @foo(i16 %c) local_unnamed_addr #0 {
+; CHECK-LABEL: define i16 @foo(
+; CHECK-SAME: i16 [[C:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    ret i16 undef
+;
 entry:
   %local1.addr = alloca i16, align 1
   store ptr %local1.addr, ptr @global, align 1
@@ -22,6 +22,14 @@ entry:
 
 ; Function Attrs: noinline nounwind writeonly
 define i16 @bar() local_unnamed_addr #1 {
+; CHECK-LABEL: define i16 @bar(
+; CHECK-SAME: ) local_unnamed_addr #[[ATTR1:[0-9]+]] {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    [[LOCAL2:%.*]] = alloca [1 x i16], align 1
+; CHECK-NEXT:    call void @llvm.lifetime.start.p0(i64 1, ptr nonnull [[LOCAL2]])
+; CHECK-NEXT:    call void @llvm.lifetime.end.p0(i64 1, ptr nonnull [[LOCAL2]])
+; CHECK-NEXT:    ret i16 undef
+;
 entry:
   %local2 = alloca [1 x i16], align 1
   call void @llvm.lifetime.start.p0(i64 1, ptr nonnull %local2)