[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
Duncan Sands
baldrick at free.fr
Mon Jul 23 23:28:10 PDT 2012
Hi Nick,
On 23/07/12 21:00, Nick Lewycky wrote:
> 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?
never happens in practice? I'm sure this is much less likely than the case of
someone storing their pointer as an integer.
> 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?
The explanation would be exactly the same...
> If there's some benefit we can get from looking at each store, please let me
> know. :)
Because you get to eliminate lots of stores, i.e. all those that don't have
pointer type?
Ciao, Duncan.
>
> 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