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

Refactor globalopt by eliminating redundant code. Fix
#64680.

Author: gandhi56

Diff for: llvm/lib/Transforms/IPO/GlobalOpt.cpp

@@ -114,55 +114,6 @@ static cl::opt<int> ColdCCRelFreq(
         "entry frequency, for a call site to be considered cold for enabling "
         "coldcc"));
 
-/// Is this global variable possibly used by a leak checker as a root?  If so,
-/// we might not really want to eliminate the stores to it.
-static bool isLeakCheckerRoot(GlobalVariable *GV) {
-  // A global variable is a root if it is a pointer, or could plausibly contain
-  // a pointer.  There are two challenges; one is that we could have a struct
-  // the has an inner member which is a pointer.  We recurse through the type to
-  // detect these (up to a point).  The other is that we may actually be a union
-  // of a pointer and another type, and so our LLVM type is an integer which
-  // gets converted into a pointer, or our type is an [i8 x #] with a pointer
-  // potentially contained here.
-
-  if (GV->hasPrivateLinkage())
-    return false;
-
-  SmallVector<Type *, 4> Types;
-  Types.push_back(GV->getValueType());
-
-  unsigned Limit = 20;
-  do {
-    Type *Ty = Types.pop_back_val();
-    switch (Ty->getTypeID()) {
-      default: break;
-      case Type::PointerTyID:
-        return true;
-      case Type::FixedVectorTyID:
-      case Type::ScalableVectorTyID:
-        if (cast<VectorType>(Ty)->getElementType()->isPointerTy())
-          return true;
-        break;
-      case Type::ArrayTyID:
-        Types.push_back(cast<ArrayType>(Ty)->getElementType());
-        break;
-      case Type::StructTyID: {
-        StructType *STy = cast<StructType>(Ty);
-        if (STy->isOpaque()) return true;
-        for (Type *InnerTy : STy->elements()) {
-          if (isa<PointerType>(InnerTy)) return true;
-          if (isa<StructType>(InnerTy) || isa<ArrayType>(InnerTy) ||
-              isa<VectorType>(InnerTy))
-            Types.push_back(InnerTy);
-        }
-        break;
-      }
-    }
-    if (--Limit == 0) return true;
-  } while (!Types.empty());
-  return false;
-}
-
 /// 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.
@@ -171,7 +122,7 @@ static bool IsSafeComputationToRemove(
   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))
@@ -193,90 +144,12 @@ 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.
-static bool
-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
-  // usage over time.  The common strategy for leak checkers is to explicitly
-  // allow the memory pointed to by globals at exit.  This is popular because it
-  // also solves another problem where the main thread of a C++ program may shut
-  // down before other threads that are still expecting to use those globals. To
-  // handle that case, we expect the program may create a singleton and never
-  // 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<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 (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();
-      Changed = true;
-    }
-  }
-
-  GV->removeDeadConstantUsers();
-  return Changed;
-}
-
 /// 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;
@@ -326,11 +199,30 @@ static bool CleanupConstantGlobalUsers(GlobalVariable *GV,
         }
       }
     } else if (StoreInst *SI = dyn_cast<StoreInst>(U)) {
-      // Store must be unreachable or storing Init into the global.
-      EraseFromParent(SI);
-    } else if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(U)) { // memset/cpy/mv
-      if (getUnderlyingObject(MI->getRawDest()) == GV)
-        EraseFromParent(MI);
+      auto *V = SI->getValueOperand();
+      if (isa<Constant>(V)) {
+        EraseFromParent(SI);
+      } else if (isa<Instruction>(V)) {
+        EraseFromParent(SI);
+        if (IsSafeComputationToRemove(V, GetTLI))
+          RecursivelyDeleteTriviallyDeadInstructions(V);
+      } else if (isa<Argument>(V)) {
+        if (!V->getType()->isPointerTy())
+          EraseFromParent(SI);
+      }
+    } else if (auto *MSI = dyn_cast<MemSetInst>(U)) { // memset/cpy/mv
+      if (getUnderlyingObject(MSI->getRawDest()) == GV)
+        EraseFromParent(MSI);
+    } else if (auto *MTI = dyn_cast<MemTransferInst>(U)) {
+      auto *Src = MTI->getRawSource();
+      auto *Dst = MTI->getRawDest();
+      if (getUnderlyingObject(Dst) != GV)
+        continue;
+      if (isa<Instruction, Operator>(Src)) {
+        EraseFromParent(MTI);
+        if (IsSafeComputationToRemove(Src, GetTLI))
+          RecursivelyDeleteTriviallyDeadInstructions(Src);
+      }
     } else if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(U)) {
       if (II->getIntrinsicID() == Intrinsic::threadlocal_address)
         append_range(WorkList, II->users());
@@ -878,12 +770,7 @@ static bool OptimizeAwayTrappingUsesOfLoads(
   // If we nuked all of the loads, then none of the stores are needed either,
   // nor is the global.
   if (AllNonStoreUsesGone) {
-    if (isLeakCheckerRoot(GV)) {
-      Changed |= CleanupPointerRootUsers(GV, GetTLI);
-    } else {
-      Changed = true;
-      CleanupConstantGlobalUsers(GV, DL);
-    }
+    Changed |= CleanupConstantGlobalUsers(GV, DL, GetTLI);
     if (GV->use_empty()) {
       LLVM_DEBUG(dbgs() << "  *** GLOBAL NOW DEAD!\n");
       Changed = true;
@@ -1497,15 +1384,7 @@ processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS,
   // Delete it now.
   if (!GS.IsLoaded) {
     LLVM_DEBUG(dbgs() << "GLOBAL NEVER LOADED: " << *GV << "\n");
-
-    if (isLeakCheckerRoot(GV)) {
-      // Delete any constant stores to the global.
-      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.
     if (GV->use_empty()) {
@@ -1529,7 +1408,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()) {
@@ -1584,7 +1463,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 "

Diff for: 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

Diff for: 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 for: llvm/test/Transforms/GlobalOpt/pr54572.ll

@@ -7,17 +7,24 @@
 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
+; CHECK: @b = internal unnamed_addr global ptr null
 ;.
 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
 }
+
+define void @neg_test(ptr %arg) {
+; CHECK-LABEL: @neg_test(
+; CHECK-NEXT:    call void @llvm.memcpy.p0.p0.i64(ptr @b, ptr [[ARG:%.*]], i64 8, i1 false)
+; CHECK-NEXT:    ret void
+;
+  call void @llvm.memcpy.p0.p0.i64(ptr @b, ptr %arg, i64 8, i1 false)
+  ret void
+}
 ;.
 ; CHECK: attributes #[[ATTR0:[0-9]+]] = { nocallback nofree nounwind willreturn memory(argmem: readwrite) }
 ;.