From 32e264921b7a683a0bf797c91c60344c8500aac2 Mon Sep 17 00:00:00 2001 From: Sterling Augustine Date: Tue, 13 Apr 2021 17:42:46 -0700 Subject: [PATCH] Revert "[GlobalOpt] Revert valgrind hacks" This reverts commit dbc16ed199dce2598f0e49943bf8354ef92a0ecc. --- llvm/lib/Transforms/IPO/GlobalOpt.cpp | 190 ++++++++++++++++++++- llvm/test/ThinLTO/X86/import-constant.ll | 3 + .../2009-11-16-BrokenPerformHeapAllocSRoA.ll | 2 +- .../GlobalOpt/cleanup-pointer-root-users.ll | 49 ++++++ .../test/Transforms/GlobalOpt/dead-store-status.ll | 4 +- 5 files changed, 239 insertions(+), 9 deletions(-) create mode 100644 llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp index 7c6c445..f275f37 100644 --- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp +++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp @@ -106,6 +106,175 @@ static cl::opt 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 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(Ty)->getElementType()->isPointerTy()) + return true; + break; + case Type::ArrayTyID: + Types.push_back(cast(Ty)->getElementType()); + break; + case Type::StructTyID: { + StructType *STy = cast(Ty); + if (STy->isOpaque()) return true; + for (StructType::element_iterator I = STy->element_begin(), + E = STy->element_end(); I != E; ++I) { + Type *InnerTy = *I; + if (isa(InnerTy)) return true; + if (isa(InnerTy) || isa(InnerTy) || + isa(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. +static bool IsSafeComputationToRemove( + Value *V, function_ref GetTLI) { + do { + if (isa(V)) + return true; + if (!V->hasOneUse()) + return false; + if (isa(V) || isa(V) || isa(V) || + isa(V)) + return false; + if (isAllocationFn(V, GetTLI)) + return true; + + Instruction *I = cast(V); + if (I->mayHaveSideEffects()) + return false; + if (GetElementPtrInst *GEP = dyn_cast(I)) { + if (!GEP->hasAllConstantIndices()) + return false; + } else if (I->getNumOperands() != 1) { + return false; + } + + V = I->getOperand(0); + } 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 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, 32> Dead; + + // Constants can't be pointers to dynamically allocated memory. + for (Value::user_iterator UI = GV->user_begin(), E = GV->user_end(); + UI != E;) { + User *U = *UI++; + if (StoreInst *SI = dyn_cast(U)) { + Value *V = SI->getValueOperand(); + if (isa(V)) { + Changed = true; + SI->eraseFromParent(); + } else if (Instruction *I = dyn_cast(V)) { + if (I->hasOneUse()) + Dead.push_back(std::make_pair(I, SI)); + } + } else if (MemSetInst *MSI = dyn_cast(U)) { + if (isa(MSI->getValue())) { + Changed = true; + MSI->eraseFromParent(); + } else if (Instruction *I = dyn_cast(MSI->getValue())) { + if (I->hasOneUse()) + Dead.push_back(std::make_pair(I, MSI)); + } + } else if (MemTransferInst *MTI = dyn_cast(U)) { + GlobalVariable *MemSrc = dyn_cast(MTI->getSource()); + if (MemSrc && MemSrc->isConstant()) { + Changed = true; + MTI->eraseFromParent(); + } else if (Instruction *I = dyn_cast(MemSrc)) { + if (I->hasOneUse()) + Dead.push_back(std::make_pair(I, MTI)); + } + } else if (ConstantExpr *CE = dyn_cast(U)) { + if (CE->use_empty()) { + CE->destroyConstant(); + Changed = true; + } + } else if (Constant *C = dyn_cast(U)) { + if (isSafeToDestroyConstant(C)) { + C->destroyConstant(); + // This could have invalidated UI, start over from scratch. + Dead.clear(); + CleanupPointerRootUsers(GV, GetTLI); + return true; + } + } + } + + 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(I->getOperand(0)); + if (!J) + break; + I->eraseFromParent(); + I = J; + } while (true); + I->eraseFromParent(); + Changed = true; + } + } + + 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. @@ -654,8 +823,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 = true; - CleanupConstantGlobalUsers(GV, nullptr, DL, GetTLI); + if (isLeakCheckerRoot(GV)) { + Changed |= CleanupPointerRootUsers(GV, GetTLI); + } else { + Changed = true; + CleanupConstantGlobalUsers(GV, nullptr, DL, GetTLI); + } if (GV->use_empty()) { LLVM_DEBUG(dbgs() << " *** GLOBAL NOW DEAD!\n"); Changed = true; @@ -1824,10 +1997,15 @@ processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS, if (!GS.IsLoaded) { LLVM_DEBUG(dbgs() << "GLOBAL NEVER LOADED: " << *GV << "\n"); - // Delete any stores we can find to the global. We may not be able to - // make it completely dead though. - Changed = - CleanupConstantGlobalUsers(GV, GV->getInitializer(), 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, GV->getInitializer(), DL, GetTLI); + } // If the global is dead now, delete it. if (GV->use_empty()) { diff --git a/llvm/test/ThinLTO/X86/import-constant.ll b/llvm/test/ThinLTO/X86/import-constant.ll index 5e8fa43..1bc2a1c 100644 --- a/llvm/test/ThinLTO/X86/import-constant.ll +++ b/llvm/test/ThinLTO/X86/import-constant.ll @@ -32,8 +32,11 @@ ; IMPORT-NEXT: @_ZL3Obj.llvm.{{.*}} = available_externally hidden constant %struct.S { i32 4, i32 8, i32* @val } ; IMPORT-NEXT: @val = available_externally global i32 42 +; OPT: @outer = internal unnamed_addr global %struct.Q zeroinitializer + ; OPT: define dso_local i32 @main() ; OPT-NEXT: entry: +; OPT-NEXT: store %struct.S* null, %struct.S** getelementptr inbounds (%struct.Q, %struct.Q* @outer, i64 0, i32 0) ; OPT-NEXT: ret i32 12 ; NOREFS: @outer = internal local_unnamed_addr global %struct.Q zeroinitializer diff --git a/llvm/test/Transforms/GlobalOpt/2009-11-16-BrokenPerformHeapAllocSRoA.ll b/llvm/test/Transforms/GlobalOpt/2009-11-16-BrokenPerformHeapAllocSRoA.ll index a997a94..461c253 100644 --- a/llvm/test/Transforms/GlobalOpt/2009-11-16-BrokenPerformHeapAllocSRoA.ll +++ b/llvm/test/Transforms/GlobalOpt/2009-11-16-BrokenPerformHeapAllocSRoA.ll @@ -17,7 +17,7 @@ define void @test() nounwind ssp { %2 = sext i32 %1 to i64 ; [#uses=1] %3 = mul i64 %2, ptrtoint (%struct.strchartype* getelementptr (%struct.strchartype, %struct.strchartype* null, i64 1) to i64) ; [#uses=1] %4 = tail call i8* @malloc(i64 %3) ; [#uses=1] -; CHECK: call i8* @malloc(i64 +; CHECK-NOT: call i8* @malloc(i64 %5 = bitcast i8* %4 to %struct.strchartype* ; <%struct.strchartype*> [#uses=1] store %struct.strchartype* %5, %struct.strchartype** @chartypes, align 8 ret void diff --git a/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll b/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll new file mode 100644 index 0000000..16da531 --- /dev/null +++ b/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll @@ -0,0 +1,49 @@ +; RUN: opt -globalopt -S -o - < %s | FileCheck %s + +@glbl = internal global i8* null + +define void @test1a() { +; CHECK-LABEL: @test1a( +; CHECK-NOT: store +; CHECK-NEXT: ret void + store i8* null, i8** @glbl + ret void +} + +define void @test1b(i8* %p) { +; CHECK-LABEL: @test1b( +; CHECK-NEXT: store +; CHECK-NEXT: ret void + store i8* %p, i8** @glbl + ret void +} + +define void @test2() { +; CHECK-LABEL: @test2( +; CHECK: alloca i8 + %txt = alloca i8 + call void @foo2(i8* %txt) + %call2 = call i8* @strdup(i8* %txt) + store i8* %call2, i8** @glbl + ret void +} +declare i8* @strdup(i8*) +declare void @foo2(i8*) + +define void @test3() uwtable personality i32 (i32, i64, i8*, i8*)* @__gxx_personality_v0 { +; CHECK-LABEL: @test3( +; CHECK-NOT: bb1: +; CHECK-NOT: bb2: +; CHECK: invoke + %ptr = invoke i8* @_Znwm(i64 1) + to label %bb1 unwind label %bb2 +bb1: + store i8* %ptr, i8** @glbl + unreachable +bb2: + %tmp1 = landingpad { i8*, i32 } + cleanup + resume { i8*, i32 } %tmp1 +} +declare i32 @__gxx_personality_v0(i32, i64, i8*, i8*) +declare i8* @_Znwm(i64) diff --git a/llvm/test/Transforms/GlobalOpt/dead-store-status.ll b/llvm/test/Transforms/GlobalOpt/dead-store-status.ll index 4721640..afd1f9f 100644 --- a/llvm/test/Transforms/GlobalOpt/dead-store-status.ll +++ b/llvm/test/Transforms/GlobalOpt/dead-store-status.ll @@ -1,10 +1,10 @@ -; RUN: opt < %s -globalopt -instcombine -S | FileCheck %s +; RUN: opt < %s -globalopt -S | FileCheck %s ; When removing the store to @global in @foo, the pass would incorrectly return ; false. This was caught by the pass return status check that is hidden under ; EXPENSIVE_CHECKS. -; CHECK-NOT: @global = internal unnamed_addr global i16* null, align 1 +; CHECK: @global = internal unnamed_addr global i16* null, align 1 ; CHECK-LABEL: @foo ; CHECK-NEXT: entry: -- 2.7.4