[PATCH] D11651: [LVI] Improve LazyValueInfo compile time performance

[Duncan P. N. Exon Smith]

> On 2015-Jul-30, at 08:45, Bruno Cardoso Lopes <bruno.cardoso at gmail.com> wrote:
> 
> bruno created this revision.
> bruno added reviewers: resistor, hfinkel, reames.
> bruno added subscribers: llvm-commits, dexonsmith.
> 
> 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 patch makes a couple of changes to speed-up LazyValueInfo and
> reduces compile time to 1m20s.
> 
> - Change the OverDefinedCache from
> DenseSet<std::pair<AssertingVH<BasicBlock>, Value*>>
> to
> DenseMap<AssertingVH<BasicBlock>, SmallPtrSet<Value *, 4>>
> 
> - Use DenseMap instead of std::map for ValueCacheEntryTy. Historically
> there seems to be some resistance regarding the change to DenseMap
> (r147980), but I couldn't find cases of iterator invalidation for
> ValueCacheEntryTy (only for ValueCache, but this one is left unchanged
> - still using std::map).
> 
> 
> http://reviews.llvm.org/D11651
> 
> Files:
>  lib/Analysis/LazyValueInfo.cpp
> 
> <D11651.31032.patch>

The `OverdefinedCache` changes LGTM, assuming you commit it
separately (this are separable, right?  If one of them causes an
invalidation bug, committing separately will make bisection more
powerful).

Regarding the `ValueCache` change:

> ===================================================================
> --- lib/Analysis/LazyValueInfo.cpp
> +++ lib/Analysis/LazyValueInfo.cpp
> @@ -315,17 +315,18 @@
>      /// This is all of the cached block information for exactly one Value*.
>      /// The entries are sorted by the BasicBlock* of the
>      /// entries, allowing us to do a lookup with a binary search.
> -    typedef std::map<AssertingVH<BasicBlock>, LVILatticeVal> ValueCacheEntryTy;
> +    typedef DenseMap<AssertingVH<BasicBlock>, LVILatticeVal> ValueCacheEntryTy;

DenseMap allocates space for 64 entries on construction...

>      /// This is all of the cached information for all values,
>      /// mapped from Value* to key information.
>      std::map<LVIValueHandle, ValueCacheEntryTy> ValueCache;
>  

Now each entry in the `ValueCache` will have a huge memory cost, even if
the `ValueCacheEntryTy::second` only has a couple of items in it.  Do
you know what the typical size is?  Would a `SmallDenseMap<>` be
appropriate?  (If they're usually close to empty, then maybe
`SmallDenseMap<..., 4>`?)