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

[Nikita Popov via llvm-commits]

================
@@ -201,61 +201,54 @@ 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.
+  while (!Writes.empty()) {
+    auto *Write = Writes.pop_back_val();
+    auto *V = Write->getOperand(0);
----------------
nikic wrote:

This is going to be the stored value for stores, but the memory intrinsics this is going to be the destination pointer. I don't think applying the check to the destination pointer makes sense? For memset we should always allow removal, for memcpy it's tricky (what really matters is the contents of the copied memory there). But in any check there would have to be on the source pointer, not the destination pointer.

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