[llvm] r245309 - [LVI] Improve LazyValueInfo compile time performance
Bruno Cardoso Lopes via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 20 11:26:39 PDT 2015
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
More information about the llvm-commits
mailing list