[llvm] 32e2649 - Revert "[GlobalOpt] Revert valgrind hacks"
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 13 22:19:04 PDT 2021
Please include some details on why a patch was reverted in the patch
description - helps with folks seeing regressions to tell if this'll
address them or someone wanting to do something similar in the future
can see what the pitfalls were.
On Tue, Apr 13, 2021 at 5:47 PM Sterling Augustine via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
>
>
> Author: Sterling Augustine
> Date: 2021-04-13T17:47:07-07:00
> New Revision: 32e264921b7a683a0bf797c91c60344c8500aac2
>
> URL: https://github.com/llvm/llvm-project/commit/32e264921b7a683a0bf797c91c60344c8500aac2
> DIFF: https://github.com/llvm/llvm-project/commit/32e264921b7a683a0bf797c91c60344c8500aac2.diff
>
> LOG: Revert "[GlobalOpt] Revert valgrind hacks"
>
> This reverts commit dbc16ed199dce2598f0e49943bf8354ef92a0ecc.
>
> Added:
> llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll
>
> Modified:
> llvm/lib/Transforms/IPO/GlobalOpt.cpp
> llvm/test/ThinLTO/X86/import-constant.ll
> llvm/test/Transforms/GlobalOpt/2009-11-16-BrokenPerformHeapAllocSRoA.ll
> llvm/test/Transforms/GlobalOpt/dead-store-status.ll
>
> Removed:
>
>
>
> ################################################################################
> diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
> index 7c6c4452b9186..f275f371c77ae 100644
> --- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp
> +++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
> @@ -106,6 +106,175 @@ 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 (StructType::element_iterator I = STy->element_begin(),
> + E = STy->element_end(); I != E; ++I) {
> + Type *InnerTy = *I;
> + 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.
> +static bool IsSafeComputationToRemove(
> + Value *V, function_ref<TargetLibraryInfo &(Function &)> GetTLI) {
> + do {
> + if (isa<Constant>(V))
> + return true;
> + if (!V->hasOneUse())
> + return false;
> + if (isa<LoadInst>(V) || isa<InvokeInst>(V) || isa<Argument>(V) ||
> + isa<GlobalValue>(V))
> + return false;
> + if (isAllocationFn(V, GetTLI))
> + return true;
> +
> + Instruction *I = cast<Instruction>(V);
> + if (I->mayHaveSideEffects())
> + return false;
> + if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(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<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;
> +
> + // 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<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>(MemSrc)) {
> + if (I->hasOneUse())
> + Dead.push_back(std::make_pair(I, MTI));
> + }
> + } else if (ConstantExpr *CE = dyn_cast<ConstantExpr>(U)) {
> + if (CE->use_empty()) {
> + CE->destroyConstant();
> + Changed = true;
> + }
> + } else if (Constant *C = dyn_cast<Constant>(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<Instruction>(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 5e8fa43d14002..1bc2a1c2f7a4d 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 a997a948b3cda..461c25316e91a 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 ; <i64> [#uses=1]
> %3 = mul i64 %2, ptrtoint (%struct.strchartype* getelementptr (%struct.strchartype, %struct.strchartype* null, i64 1) to i64) ; <i64> [#uses=1]
> %4 = tail call i8* @malloc(i64 %3) ; <i8*> [#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 0000000000000..16da5315db0c3
> --- /dev/null
> +++ b/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll
> @@ -0,0 +1,49 @@
> +; RUN: opt -globalopt -S -o - < %s | FileCheck %s
> +
> + at 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 472164006eea8..afd1f9f6cabfd 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:
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list