Revert "[GlobalOpt] Handle operators separately when removing GV users"

[efriedma-quic]

Reverts #84694 . Review was incomplete.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Eli Friedman (efriedma-quic)

Changes

Reverts llvm/llvm-project#84694 . Review was incomplete.

---

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

4 Files Affected:

• (modified) llvm/lib/Transforms/IPO/GlobalOpt.cpp (+153-32)
• (modified) llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users-gep-constexpr.ll (+1)
• (modified) llvm/test/Transforms/GlobalOpt/dead-store-status.ll (+2)
• (modified) llvm/test/Transforms/GlobalOpt/pr54572.ll (+3-10)

diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
index 7b7b3802d7a77..2d046f09f1b2b 100644
--- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -114,6 +114,55 @@ 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.
@@ -122,7 +171,7 @@ static bool IsSafeComputationToRemove(
   do {
     if (isa<Constant>(V))
       return true;
-    if (V->hasNUsesOrMore(1))
+    if (!V->hasOneUse())
       return false;
     if (isa<LoadInst>(V) || isa<InvokeInst>(V) || isa<Argument>(V) ||
         isa<GlobalValue>(V))
@@ -144,12 +193,90 @@ 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,
-    function_ref<TargetLibraryInfo &(Function &)> GetTLI) {
+static bool CleanupConstantGlobalUsers(GlobalVariable *GV,
+                                       const DataLayout &DL) {
   Constant *Init = GV->getInitializer();
   SmallVector<User *, 8> WorkList(GV->users());
   SmallPtrSet<User *, 8> Visited;
@@ -199,30 +326,11 @@ static bool CleanupConstantGlobalUsers(
         }
       }
     } else if (StoreInst *SI = dyn_cast<StoreInst>(U)) {
-      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);
-      }
+      // 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);
     } else if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(U)) {
       if (II->getIntrinsicID() == Intrinsic::threadlocal_address)
         append_range(WorkList, II->users());
@@ -770,7 +878,12 @@ static bool OptimizeAwayTrappingUsesOfLoads(
   // If we nuked all of the loads, then none of the stores are needed either,
   // nor is the global.
   if (AllNonStoreUsesGone) {
-    Changed |= CleanupConstantGlobalUsers(GV, DL, GetTLI);
+    if (isLeakCheckerRoot(GV)) {
+      Changed |= CleanupPointerRootUsers(GV, GetTLI);
+    } else {
+      Changed = true;
+      CleanupConstantGlobalUsers(GV, DL);
+    }
     if (GV->use_empty()) {
       LLVM_DEBUG(dbgs() << "  *** GLOBAL NOW DEAD!\n");
       Changed = true;
@@ -1384,7 +1497,15 @@ processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS,
   // Delete it now.
   if (!GS.IsLoaded) {
     LLVM_DEBUG(dbgs() << "GLOBAL NEVER LOADED: " << *GV << "\n");
-    Changed = CleanupConstantGlobalUsers(GV, DL, GetTLI);
+
+    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);
+    }
 
     // If the global is dead now, delete it.
     if (GV->use_empty()) {
@@ -1408,7 +1529,7 @@ processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS,
     }
 
     // Clean up any obviously simplifiable users now.
-    Changed |= CleanupConstantGlobalUsers(GV, DL, GetTLI);
+    Changed |= CleanupConstantGlobalUsers(GV, DL);
 
     // If the global is dead now, just nuke it.
     if (GV->use_empty()) {
@@ -1463,7 +1584,7 @@ processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS,
       }
 
       // Clean up any obviously simplifiable users now.
-      CleanupConstantGlobalUsers(GV, DL, GetTLI);
+      CleanupConstantGlobalUsers(GV, DL);
 
       if (GV->use_empty()) {
         LLVM_DEBUG(dbgs() << "   *** Substituting initializer allowed us to "
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 66a99c15ba4d3..26728a74d032c 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,6 +99,7 @@ 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 --git a/llvm/test/Transforms/GlobalOpt/dead-store-status.ll b/llvm/test/Transforms/GlobalOpt/dead-store-status.ll
index 597c08929af90..9a8fbb8d65f0e 100644
--- a/llvm/test/Transforms/GlobalOpt/dead-store-status.ll
+++ b/llvm/test/Transforms/GlobalOpt/dead-store-status.ll
@@ -4,6 +4,8 @@
 ; 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 50f3ce92e104b..962c75bd83b41 100644
--- a/llvm/test/Transforms/GlobalOpt/pr54572.ll
+++ b/llvm/test/Transforms/GlobalOpt/pr54572.ll
@@ -7,24 +7,17 @@
 declare void @llvm.memcpy.p0.p0.i64(ptr noalias nocapture writeonly, ptr noalias nocapture readonly, i64, i1 immarg)
 
 ;.
-; CHECK: @b = internal unnamed_addr global ptr null
+; 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
 }
-
-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) }
 ;.

[gandhi56]

I thought I saw an approval from someone, I am sorry about that.