[llvm] r245309 - [LVI] Improve LazyValueInfo compile time performance
Yaron Keren via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 19 14:08:09 PDT 2015
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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150820/61d2db77/attachment.html>
More information about the llvm-commits
mailing list