[llvm-commits] [llvm] r160602 - in /llvm/trunk: lib/Transforms/IPO/GlobalOpt.cpp test/Transforms/GlobalOpt/2009-11-16-BrokenPerformHeapAllocSRoA.ll test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll
Nick Lewycky
nicholas at mxc.ca
Mon Jul 23 12:00:42 PDT 2012
Duncan Sands wrote:
> Hi Nick,
>
>> Teach globalopt to play nice with leak checkers. This is a reapplication of
>> r160529 that was subsequently reverted. The fix was to not call
>> GV->eraseFromParent() right before the caller does the same. The existing
>> testcases already caught this bug if run under valgrind.
>
> rather than scanning the variable looking for pointers in it, why not just
> remove stores as before, but not remove stores of values of pointer type?
What if the value is a first-class aggregate structure containing a
pointer member?
The other issue is that this way we'd need to look at all the stores on
the GV instead of looking at the single GV. Now, technically GlobalOpt
already has to do that anyway and I could add a new enum to StoredType
to indicate that it's stored to once but not with a pointer, but I don't
see what makes that an improvement. Looking at the GV has the nice
property that I can explain to users why their global wasn't being
treated as a pointer root -- have you tried making your pointer root GV
have pointer type?
If there's some benefit we can get from looking at each store, please
let me know. :)
Nick
> Ciao, Duncan.
>
>>
>> Added:
>> llvm/trunk/test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll
>> Modified:
>> llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp
>> llvm/trunk/test/Transforms/GlobalOpt/2009-11-16-BrokenPerformHeapAllocSRoA.ll
>>
>> Modified: llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp?rev=160602&r1=160601&r2=160602&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp (original)
>> +++ llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp Sat Jul 21 03:29:45 2012
>> @@ -296,6 +296,165 @@
>> return false;
>> }
>>
>> +/// isLeakCheckerRoot - 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(cast<PointerType>(GV->getType())->getElementType());
>> +
>> + unsigned Limit = 20;
>> + do {
>> + Type *Ty = Types.pop_back_val();
>> + switch (Ty->getTypeID()) {
>> + default: break;
>> + case Type::PointerTyID: return true;
>> + case Type::ArrayTyID:
>> + case Type::VectorTyID: {
>> + SequentialType *STy = cast<SequentialType>(Ty);
>> + Types.push_back(STy->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<CompositeType>(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) {
>> + do {
>> + if (isa<Constant>(V))
>> + return true;
>> + if (!V->hasOneUse())
>> + return false;
>> + if (isa<LoadInst>(V) || isa<Argument>(V) || isa<GlobalValue>(V))
>> + return false;
>> + if (isAllocationFn(V))
>> + 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 (1);
>> +}
>> +
>> +/// CleanupPointerRootUsers - 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) {
>> + // 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 whitelist 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::use_iterator UI = GV->use_begin(), E = GV->use_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 (SafeToDestroyConstant(C)) {
>> + C->destroyConstant();
>> + // This could have invalidated UI, start over from scratch.
>> + Dead.clear();
>> + CleanupPointerRootUsers(GV);
>> + return true;
>> + }
>> + }
>> + }
>> +
>> + for (int i = 0, e = Dead.size(); i != e; ++i) {
>> + if (IsSafeComputationToRemove(Dead[i].first)) {
>> + Dead[i].second->eraseFromParent();
>> + Instruction *I = Dead[i].first;
>> + do {
>> + Instruction *J = dyn_cast<Instruction>(I->getOperand(0));
>> + I->eraseFromParent();
>> + if (!J)
>> + break;
>> + I = J;
>> + } while (!isAllocationFn(I));
>> + I->eraseFromParent();
>> + }
>> + }
>> +
>> + return Changed;
>> +}
>> +
>> /// CleanupConstantGlobalUsers - 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
>> @@ -812,13 +971,18 @@
>> // If we nuked all of the loads, then none of the stores are needed either,
>> // nor is the global.
>> if (AllNonStoreUsesGone) {
>> - DEBUG(dbgs()<< " *** GLOBAL NOW DEAD!\n");
>> - CleanupConstantGlobalUsers(GV, 0, TD, TLI);
>> + if (isLeakCheckerRoot(GV)) {
>> + Changed |= CleanupPointerRootUsers(GV);
>> + } else {
>> + Changed = true;
>> + CleanupConstantGlobalUsers(GV, 0, TD, TLI);
>> + }
>> if (GV->use_empty()) {
>> + DEBUG(dbgs()<< " *** GLOBAL NOW DEAD!\n");
>> + Changed = true;
>> GV->eraseFromParent();
>> ++NumDeleted;
>> }
>> - Changed = true;
>> }
>> return Changed;
>> }
>> @@ -1794,10 +1958,15 @@
>> if (!GS.isLoaded) {
>> DEBUG(dbgs()<< "GLOBAL NEVER LOADED: "<< *GV);
>>
>> - // Delete any stores we can find to the global. We may not be able to
>> - // make it completely dead though.
>> - bool Changed = CleanupConstantGlobalUsers(GV, GV->getInitializer(),
>> - TD, TLI);
>> + bool Changed;
>> + if (isLeakCheckerRoot(GV)) {
>> + // Delete any constant stores to the global.
>> + Changed = CleanupPointerRootUsers(GV);
>> + } 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(), TD, TLI);
>> + }
>>
>> // If the global is dead now, delete it.
>> if (GV->use_empty()) {
>> @@ -1845,7 +2014,7 @@
>>
>> if (GV->use_empty()) {
>> DEBUG(dbgs()<< " *** Substituting initializer allowed us to"
>> -<< "simplify all users and delete global!\n");
>> +<< "simplify all users and delete global!\n");
>> GV->eraseFromParent();
>> ++NumDeleted;
>> } else {
>>
>> Modified: llvm/trunk/test/Transforms/GlobalOpt/2009-11-16-BrokenPerformHeapAllocSRoA.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GlobalOpt/2009-11-16-BrokenPerformHeapAllocSRoA.ll?rev=160602&r1=160601&r2=160602&view=diff
>> ==============================================================================
>> --- llvm/trunk/test/Transforms/GlobalOpt/2009-11-16-BrokenPerformHeapAllocSRoA.ll (original)
>> +++ llvm/trunk/test/Transforms/GlobalOpt/2009-11-16-BrokenPerformHeapAllocSRoA.ll Sat Jul 21 03:29:45 2012
>> @@ -17,7 +17,7 @@
>> %2 = sext i32 %1 to i64 ;<i64> [#uses=1]
>> %3 = mul i64 %2, ptrtoint (%struct.strchartype* getelementptr (%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
>>
>> Added: llvm/trunk/test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll?rev=160602&view=auto
>> ==============================================================================
>> --- llvm/trunk/test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll (added)
>> +++ llvm/trunk/test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll Sat Jul 21 03:29:45 2012
>> @@ -0,0 +1,20 @@
>> +; RUN: opt -globalopt -S -o -< %s | FileCheck %s
>> +
>> + at test1 = internal global i8* null
>> +
>> +define void @test1a() {
>> +; CHECK: @test1a
>> +; CHECK-NOT: store
>> +; CHECK-NEXT: ret void
>> + store i8* null, i8** @test1
>> + ret void
>> +}
>> +
>> +define void @test1b(i8* %p) {
>> +; CHECK: @test1b
>> +; CHECK-NEXT: store
>> +; CHECK-NEXT: ret void
>> + store i8* %p, i8** @test1
>> + ret void
>> +}
>> +
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list