[PATCH] D70376: [LVI] Restructure caching

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70376