Author: Eli Friedman Date: 2025-03-25T11:17:45-07:00 New Revision: 244daaf51c11c04971f6adb144bbfacf4074b1a8
URL: https://github.com/llvm/llvm-project/commit/244daaf51c11c04971f6adb144bbfacf4074b1a8 DIFF: https://github.com/llvm/llvm-project/commit/244daaf51c11c04971f6adb144bbfacf4074b1a8.diff LOG: Revert "[GlobalOpt] Handle operators separately when removing GV users (#84694)" This reverts commit 51dad714e82e3e15c339aade8be605ed09bbabab. Added: Modified: llvm/lib/Transforms/IPO/GlobalOpt.cpp llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users-gep-constexpr.ll llvm/test/Transforms/GlobalOpt/dead-store-status.ll llvm/test/Transforms/GlobalOpt/pr54572.ll Removed: ################################################################################ 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) } ;. _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
