[llvm] dbc16ed - [GlobalOpt] Revert valgrind hacks
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 13 22:29:46 PDT 2021
Please include more detail in commit messages - it looks like all the
detail from the phab review wasn't included/was lost here.
On Tue, Apr 13, 2021 at 9:11 AM Evgeny Leviant via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
>
>
> Author: Evgeny Leviant
> Date: 2021-04-13T19:11:10+03:00
> New Revision: dbc16ed199dce2598f0e49943bf8354ef92a0ecc
>
> URL: https://github.com/llvm/llvm-project/commit/dbc16ed199dce2598f0e49943bf8354ef92a0ecc
> DIFF: https://github.com/llvm/llvm-project/commit/dbc16ed199dce2598f0e49943bf8354ef92a0ecc.diff
>
> LOG: [GlobalOpt] Revert valgrind hacks
>
> Differential revision: https://reviews.llvm.org/D69428
>
> Added:
>
>
> 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/cleanup-pointer-root-users.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 f275f371c77ae..7c6c4452b9186 100644
> --- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp
> +++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
> @@ -106,175 +106,6 @@ 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.
> @@ -823,12 +654,8 @@ static bool OptimizeAwayTrappingUsesOfLoads(
> // If we nuked all of the loads, then none of the stores are needed either,
> // nor is the global.
> if (AllNonStoreUsesGone) {
> - if (isLeakCheckerRoot(GV)) {
> - Changed |= CleanupPointerRootUsers(GV, GetTLI);
> - } else {
> - Changed = true;
> - CleanupConstantGlobalUsers(GV, nullptr, DL, GetTLI);
> - }
> + Changed = true;
> + CleanupConstantGlobalUsers(GV, nullptr, DL, GetTLI);
> if (GV->use_empty()) {
> LLVM_DEBUG(dbgs() << " *** GLOBAL NOW DEAD!\n");
> Changed = true;
> @@ -1997,15 +1824,10 @@ processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS,
> if (!GS.IsLoaded) {
> LLVM_DEBUG(dbgs() << "GLOBAL NEVER LOADED: " << *GV << "\n");
>
> - 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);
> - }
> + // 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 1bc2a1c2f7a4d..5e8fa43d14002 100644
> --- a/llvm/test/ThinLTO/X86/import-constant.ll
> +++ b/llvm/test/ThinLTO/X86/import-constant.ll
> @@ -32,11 +32,8 @@
> ; 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 461c25316e91a..a997a948b3cda 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-NOT: call i8* @malloc(i64
> +; CHECK: 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
> index 16da5315db0c3..e69de29bb2d1d 100644
> --- a/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll
> +++ b/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll
> @@ -1,49 +0,0 @@
> -; 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 afd1f9f6cabfd..472164006eea8 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 -S | FileCheck %s
> +; RUN: opt < %s -globalopt -instcombine -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: @global = internal unnamed_addr global i16* null, align 1
> +; CHECK-NOT: @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