[llvm] r245309 - [LVI] Improve LazyValueInfo compile time performance
Bruno Cardoso Lopes via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 21 14:20:54 PDT 2015
Silly me!
Committed r245739!
Thanks,
On Thu, Aug 20, 2015 at 5:00 PM, Yaron Keren <yaron.keren at gmail.com> wrote:
> Thanks!
> Minor nitpick, ClearSet could be ClearVector, a SmallVector as it's only
> accessed sequentially.
>
> 2015-08-20 21:26 GMT+03:00 Bruno Cardoso Lopes <bruno.cardoso at gmail.com>:
>>
>> Hi Yaron,
>>
>> Thanks for the catch :-)
>> Committed a fix in r245590.
>>
>> Cheers,
>>
>> On Wed, Aug 19, 2015 at 6:08 PM, Yaron Keren <yaron.keren at gmail.com>
>> wrote:
>> > Hi Bruno,
>> >
>> > I believe there is use-after-free and possibly use of dangling iterators
>> > in
>> > this code.
>> > Please follow the first iteration of the while loop:
>> >
>> > while (!worklist.empty()) {
>> > BasicBlock *ToUpdate = worklist.back();
>> > worklist.pop_back();
>> > ...
>> >
>> > upon entry, ToUpdate == OldSucc which was pushed prior to entering the
>> > while
>> > loop.
>> > Thus, ClearSet == ValueSet, V is from ClearSet so ValueSet.count(V) must
>> > be
>> > 1, and the erase command will always happen on the first iteration
>> >
>> > ValueSet.erase(V);
>> >
>> > since ClearSet == ValueSet this commands will modify ClearSet which was
>> > used
>> > in the ranged for loop.
>> > That is likely to be incorrect if erase invalidates ClearSet end()
>> > iterator
>> > (which is computed once only at the start of ranged for loop) or the
>> > SmallPtrSet iterator itself.
>> >
>> > Even if the loop continues correctly, should the set becomes empty, it's
>> > completely erased and freed when the DenseMap entry is erased
>> >
>> > if (ValueSet.empty())
>> > OverDefinedCache.erase(OI);
>> >
>> > and the test condition of the ranged for loop I != E will access dead
>> > and
>> > freed memory just before exiting the loop.
>> >
>> > I do see such use-after-free report from asan on a complicated example
>> > case.
>> >
>> > Yaron
>> >
>> >
>> > 2015-08-18 19:34 GMT+03:00 Bruno Cardoso Lopes via llvm-commits
>> > <llvm-commits at lists.llvm.org>:
>> >>
>> >> Author: bruno
>> >> Date: Tue Aug 18 11:34:27 2015
>> >> New Revision: 245309
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=245309&view=rev
>> >> Log:
>> >> [LVI] Improve LazyValueInfo compile time performance
>> >>
>> >> Changes in LoopUnroll in the past six months exposed scalability
>> >> issues in LazyValueInfo when used from JumpThreading. One internal test
>> >> that used to take 20s under -O2 now takes 6min.
>> >>
>> >> This commit change the OverDefinedCache from
>> >> DenseSet<std::pair<AssertingVH<BasicBlock>, Value*>> to
>> >> DenseMap<AssertingVH<BasicBlock>, SmallPtrSet<Value *, 4>>
>> >> and reduces compile time down to 1m40s.
>> >>
>> >> Differential Revision: http://reviews.llvm.org/D11651
>> >>
>> >> rdar://problem/21320066
>> >>
>> >> Modified:
>> >> llvm/trunk/lib/Analysis/LazyValueInfo.cpp
>> >>
>> >> Modified: llvm/trunk/lib/Analysis/LazyValueInfo.cpp
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/LazyValueInfo.cpp?rev=245309&r1=245308&r2=245309&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- llvm/trunk/lib/Analysis/LazyValueInfo.cpp (original)
>> >> +++ llvm/trunk/lib/Analysis/LazyValueInfo.cpp Tue Aug 18 11:34:27 2015
>> >> @@ -324,8 +324,9 @@ namespace {
>> >> /// This tracks, on a per-block basis, the set of values that are
>> >> /// over-defined at the end of that block. This is required
>> >> /// for cache updating.
>> >> - typedef std::pair<AssertingVH<BasicBlock>, Value*>
>> >> OverDefinedPairTy;
>> >> - DenseSet<OverDefinedPairTy> OverDefinedCache;
>> >> + typedef DenseMap<AssertingVH<BasicBlock>, SmallPtrSet<Value *, 4>>
>> >> + OverDefinedCacheTy;
>> >> + OverDefinedCacheTy OverDefinedCache;
>> >>
>> >> /// Keep track of all blocks that we have ever seen, so we
>> >> /// don't spend time removing unused blocks from our caches.
>> >> @@ -359,7 +360,7 @@ namespace {
>> >> SeenBlocks.insert(BB);
>> >> lookup(Val)[BB] = Result;
>> >> if (Result.isOverdefined())
>> >> - OverDefinedCache.insert(std::make_pair(BB, Val));
>> >> + OverDefinedCache[BB].insert(Val);
>> >> }
>> >>
>> >> LVILatticeVal getBlockValue(Value *Val, BasicBlock *BB);
>> >> @@ -425,14 +426,16 @@ namespace {
>> >> } // end anonymous namespace
>> >>
>> >> void LVIValueHandle::deleted() {
>> >> - typedef std::pair<AssertingVH<BasicBlock>, Value*>
>> >> OverDefinedPairTy;
>> >> -
>> >> - SmallVector<OverDefinedPairTy, 4> ToErase;
>> >> - for (const OverDefinedPairTy &P : Parent->OverDefinedCache)
>> >> - if (P.second == getValPtr())
>> >> - ToErase.push_back(P);
>> >> - for (const OverDefinedPairTy &P : ToErase)
>> >> - Parent->OverDefinedCache.erase(P);
>> >> + SmallVector<AssertingVH<BasicBlock>, 4> ToErase;
>> >> + for (auto &I : Parent->OverDefinedCache) {
>> >> + SmallPtrSetImpl<Value *> &ValueSet = I.second;
>> >> + if (ValueSet.count(getValPtr()))
>> >> + ValueSet.erase(getValPtr());
>> >> + if (ValueSet.empty())
>> >> + ToErase.push_back(I.first);
>> >> + }
>> >> + for (auto &BB : ToErase)
>> >> + Parent->OverDefinedCache.erase(BB);
>> >>
>> >> // This erasure deallocates *this, so it MUST happen after we're
>> >> done
>> >> // using any and all members of *this.
>> >> @@ -446,15 +449,11 @@ void LazyValueInfoCache::eraseBlock(Basi
>> >> return;
>> >> SeenBlocks.erase(I);
>> >>
>> >> - SmallVector<OverDefinedPairTy, 4> ToErase;
>> >> - for (const OverDefinedPairTy& P : OverDefinedCache)
>> >> - if (P.first == BB)
>> >> - ToErase.push_back(P);
>> >> - for (const OverDefinedPairTy &P : ToErase)
>> >> - OverDefinedCache.erase(P);
>> >> + auto ODI = OverDefinedCache.find(BB);
>> >> + if (ODI != OverDefinedCache.end())
>> >> + OverDefinedCache.erase(ODI);
>> >>
>> >> - for (std::map<LVIValueHandle, ValueCacheEntryTy>::iterator
>> >> - I = ValueCache.begin(), E = ValueCache.end(); I != E; ++I)
>> >> + for (auto I = ValueCache.begin(), E = ValueCache.end(); I != E; ++I)
>> >> I->second.erase(BB);
>> >> }
>> >>
>> >> @@ -483,8 +482,7 @@ bool LazyValueInfoCache::hasBlockValue(V
>> >> return true;
>> >>
>> >> LVIValueHandle ValHandle(Val, this);
>> >> - std::map<LVIValueHandle, ValueCacheEntryTy>::iterator I =
>> >> - ValueCache.find(ValHandle);
>> >> + auto I = ValueCache.find(ValHandle);
>> >> if (I == ValueCache.end()) return false;
>> >> return I->second.count(BB);
>> >> }
>> >> @@ -1053,10 +1051,10 @@ void LazyValueInfoCache::threadEdge(Basi
>> >> std::vector<BasicBlock*> worklist;
>> >> worklist.push_back(OldSucc);
>> >>
>> >> - DenseSet<Value*> ClearSet;
>> >> - for (OverDefinedPairTy &P : OverDefinedCache)
>> >> - if (P.first == OldSucc)
>> >> - ClearSet.insert(P.second);
>> >> + auto I = OverDefinedCache.find(OldSucc);
>> >> + if (I == OverDefinedCache.end())
>> >> + return; // Nothing to process here.
>> >> + SmallPtrSetImpl<Value *> &ClearSet = I->second;
>> >>
>> >> // Use a worklist to perform a depth-first search of OldSucc's
>> >> successors.
>> >> // NOTE: We do not need a visited list since any blocks we have
>> >> already
>> >> @@ -1072,9 +1070,12 @@ void LazyValueInfoCache::threadEdge(Basi
>> >> bool changed = false;
>> >> for (Value *V : ClearSet) {
>> >> // If a value was marked overdefined in OldSucc, and is here
>> >> too...
>> >> - DenseSet<OverDefinedPairTy>::iterator OI =
>> >> - OverDefinedCache.find(std::make_pair(ToUpdate, V));
>> >> - if (OI == OverDefinedCache.end()) continue;
>> >> + auto OI = OverDefinedCache.find(ToUpdate);
>> >> + if (OI == OverDefinedCache.end())
>> >> + continue;
>> >> + SmallPtrSetImpl<Value *> &ValueSet = OI->second;
>> >> + if (!ValueSet.count(V))
>> >> + continue;
>> >>
>> >> // Remove it from the caches.
>> >> ValueCacheEntryTy &Entry = ValueCache[LVIValueHandle(V, this)];
>> >> @@ -1082,7 +1083,9 @@ void LazyValueInfoCache::threadEdge(Basi
>> >>
>> >> assert(CI != Entry.end() && "Couldn't find entry to update?");
>> >> Entry.erase(CI);
>> >> - OverDefinedCache.erase(OI);
>> >> + ValueSet.erase(V);
>> >> + if (ValueSet.empty())
>> >> + OverDefinedCache.erase(OI);
>> >>
>> >> // If we removed anything, then we potentially need to update
>> >> // blocks successors too.
>> >>
>> >>
>> >> _______________________________________________
>> >> llvm-commits mailing list
>> >> llvm-commits at lists.llvm.org
>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>> >
>> >
>>
>>
>>
>> --
>> Bruno Cardoso Lopes
>> http://www.brunocardoso.cc
>
>
--
Bruno Cardoso Lopes
http://www.brunocardoso.cc
More information about the llvm-commits
mailing list