Page MenuHomePhabricator

[LVI] Restructure caching
ClosedPublic

Authored by nikic on Mon, Nov 18, 12:59 AM.

Details

Summary

Variant on D70103. The caching is switched to always use a BB to cache entry map, which then contains per-value caches. A separate set contains value handles with a deletion callback. This allows us to properly invalidate overdefined values.

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, but possibly that's not the right choice.

Diff Detail

Event Timeline

nikic created this revision.Mon, Nov 18, 12:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Nov 18, 12:59 AM
efriedma accepted this revision.Mon, Nov 18, 4:36 PM

Simpler is good, I think. And I like the use of AssertingVH. LGTM.

It would be nice to have a testcase for PR43909? But not critical, I think; the expanded use of AssertingVH should catch any future issues here.

This revision is now accepted and ready to land.Mon, Nov 18, 4:36 PM
nikic added a subscriber: reames.

@efriedma When using AssertingVH a bunch of the existing JT/CVP tests start to fail, so I think we don't need a test case that produces an actual collision in the map.

Adding @reames in case he has some thoughts on how we handle caching/invalidation.

uenoku added a subscriber: uenoku.Tue, Nov 19, 4:59 AM
fhahn added a subscriber: fhahn.Tue, Nov 19, 8:40 AM
jmorse added a subscriber: jmorse.Thu, Nov 28, 2:06 AM
reames added a comment.Mon, Dec 2, 5:15 PM

Adding @reames in case he has some thoughts on how we handle caching/invalidation.

This looks very reasonable to me. I ran through it and didn't spot anything obviously problematic, so also LGTM.

nikic updated this revision to Diff 232150.Wed, Dec 4, 8:38 AM

Don't use AssertingVH for BasicBlock. See D29006 for why this needs to be a PoisoningVH. I believe using AssertingVH should be fine for the Value case though, as have a CallbackVH that should *always* remove these.

This revision was automatically updated to reflect the committed changes.