[llvm] r245309 - [LVI] Improve LazyValueInfo compile time performance
Yaron Keren via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 20 13:00:15 PDT 2015
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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150820/fd0eb588/attachment.html>
More information about the llvm-commits
mailing list