[PATCH] D70376: [LVI] Restructure caching

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D70376#2145825 <https://reviews.llvm.org/D70376#2145825>, @nikic wrote:

> In D70376#2145225 <https://reviews.llvm.org/D70376#2145225>, @tejohnson wrote:
>
> > Hi @nikic, I just tracked down a big compile time increase to this patch. The issue cropped up for a very large function, where a cycle profile showed hotspots in:
> >
> > llvm::DenseMapBase<llvm::SmallDenseMap<llvm::AssertingVH<llvm::Value>, llvm::ValueLatticeElement, 4u, llvm::DenseMapInfo<llvm::AssertingVH<llvm::Value> >, llvm::detail::DenseMapPair<llvm::AssertingVH<llvm::Value>, llvm::ValueLatticeElement> >, llvm::AssertingVH<llvm::Value>, llvm::ValueLatticeElement, llvm::DenseMapInfo<llvm::AssertingVH<llvm::Value> >, llvm::detail::DenseMapPair<llvm::AssertingVH<llvm::Value>, llvm::ValueLatticeElement> >::erase(llvm::AssertingVH<llvm::Value> const&)
> >
> > and
> >
> > (anonymous namespace)::LVIValueHandle::deleted()
> >
> > The problem is related to this patch's restructuring of the LazyValueInfoCache so that instead of a 2-level map from Value* -> BB -> ValueLatticeElement, it now has a 2-level map from BB -> Value -> ValueLatticeElement. The problem is that LVIValueHandle::deleted invokes LazyValueInfoCache::eraseValue on a Value*, which now needs to walk through every entry in the outer map to remove the Value* from every block containing it in its lattice. Before, it could simply do a single lookup on the outer map to remove the Value.
> >
> > When I revert this patch the compile time goes down ~35%.
> >
> > I noticed in your description this comment:
> >  "A possible alternative would be to always cache by value first and have per-BB maps/sets in the each cache entry. In that case we could use a ValueMap and would avoid the separate value handle set. I went with the BB indexing at the top level to make it easier to integrate D69914 <https://reviews.llvm.org/D69914>, but possibly that's not the right choice."
> >
> > It sounds like your proposed alternative would address this issue. Would you be able to do that?
>
>
> Unfortunately the alternative approach has its own issues. It would fix this performance problem, but I think it would also raise memory usage significantly. D81811 <https://reviews.llvm.org/D81811> might be a good way to go about it, because it removes the need for separate tracking of overdefined values, which would fix the non-determinism problem here as a side-effect.
>
> Is it possible to share the problematic test case, so I can evaluate different approaches?


Will work on getting you a test case today. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70376/new/

https://reviews.llvm.org/D70376