[GlobalOpt] Refactor CleanupPointerRootUsers

Currently the algorithm combines removal of stores
and store operands to GV in a single loop. This patch
refines the current algorithm to account for certain
edge cases.

Fixes #64680.

Author: gandhi56

llvm/lib/Transforms/IPO/GlobalOpt.cpp

@@ -201,61 +201,51 @@ 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());
     }
   }
 
-  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);
+    if (isa<Constant>(V)) {
       Changed = true;
+      Write->eraseFromParent();
+    } else if (isa<Instruction>(V)) {
+      if (IsSafeComputationToRemove(V, GetTLI)) {
+        Changed = true;
+        Write->eraseFromParent();
+        RemoveComputationChain(cast<Instruction>(V));
+      } else {
+        Changed = true;
+        Write->eraseFromParent();
+      }
     }
   }
 

llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-stores.ll

@@ -0,0 +1,62 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt -passes=globalopt < %s -S | FileCheck %s
+
+@a = internal unnamed_addr global i32 0, align 4
+@b = internal unnamed_addr global [3 x ptr] zeroinitializer, align 16
+
+define i32 @main() local_unnamed_addr {
+; CHECK-LABEL: define i32 @main() local_unnamed_addr {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[E:%.*]] = alloca i32, align 4
+; CHECK-NEXT:    [[DOTPR:%.*]] = load i32, ptr @a, align 4
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp slt i32 [[DOTPR]], 3
+; CHECK-NEXT:    br i1 [[CMP1]], label [[FOR_BODY:%.*]], label [[FOR_END:%.*]]
+; CHECK:       for.body:
+; CHECK-NEXT:    store i32 8, ptr [[E]], align 4
+; CHECK-NEXT:    call void @bar20_()
+; CHECK-NEXT:    [[TMP0:%.*]] = load i32, ptr [[E]], align 4
+; CHECK-NEXT:    [[TOBOOL_NOT:%.*]] = icmp eq i32 [[TMP0]], 0
+; CHECK-NEXT:    br i1 [[TOBOOL_NOT]], label [[IF_THEN:%.*]], label [[IF_END:%.*]]
+; CHECK:       if.then:
+; CHECK-NEXT:    call void @foo()
+; CHECK-NEXT:    br label [[IF_END]]
+; CHECK:       if.end:
+; 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
+; CHECK-NEXT:    [[CMP:%.*]] = icmp slt i32 [[TMP1]], 2
+; CHECK-NEXT:    br i1 [[CMP]], label [[FOR_BODY]], label [[FOR_END]]
+; CHECK:       for.end:
+; CHECK-NEXT:    ret i32 0
+;
+entry:
+  %e = alloca i32, align 4
+  %.pr = load i32, ptr @a, align 4
+  %cmp1 = icmp slt i32 %.pr, 3
+  br i1 %cmp1, label %for.body, label %for.end
+
+for.body:                                         ; preds = %entry, %if.end
+  store i32 8, ptr %e, align 4
+  call void @bar20_()
+  %0 = load i32, ptr %e, align 4
+  %tobool.not = icmp eq i32 %0, 0
+  br i1 %tobool.not, label %if.then, label %if.end
+
+if.then:                                          ; preds = %for.body
+  call void @foo()
+  br label %if.end
+
+if.end:                                           ; preds = %if.then, %for.body
+  store ptr %e, ptr getelementptr inbounds ([3 x ptr], ptr @b, i64 0, i64 2), align 16
+  %1 = load i32, ptr @a, align 4
+  %inc = add nsw i32 %1, 1
+  store i32 %inc, ptr @a, align 4
+  %cmp = icmp slt i32 %1, 2
+  br i1 %cmp, label %for.body, label %for.end
+
+for.end:                                          ; preds = %if.end, %entry
+  ret i32 0
+}
+
+declare void @bar20_() local_unnamed_addr
+declare void @foo() local_unnamed_addr

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