Limit alias regions in Wasm-to-CLIF translation to 256 - #14253
Conversation
github-actions
Bot
added
the
cranelift
|
This fixes the problem I reported in #14210. I used the generator script from that issue and ran
Time roughly doubles when N doubles, out to N=8000. The blowup is gone and my FYI, the RPO worklist from #14211 is still 3-5x faster on top of this PR One request: if this misses the 49.0.0 branch on the 5th, could it be |
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
Review on the last commit only (the lattice commit still has work in progress as I understand it from our discussion yesterday, re: scoped hashmap and such).
My main concern with the machinery here is the description-string complexity. Seeing the "reverse map" that keeps original identities around, and especially the string-joining machinery, inside Cranelift itself, both (i) raises serious efficiency concerns for me (we may be at a net speedup by preventing alias-analysis state blowup, but we are still introducing a lot of string munging to produce strings that most compilations will never need), and (ii) seems more complex than it needs to be.
I wonder if we could simply drop the descriptions, and instead emit log::trace lines for mapping of alias-region keys to Cranelift region numbers, and let that suffice for debugging? It seems that all the same information would be there, but we wouldn't be eagerly computing then throwing it away on every compilation...
There were two ways in which alias analysis's `LastStores` state was not a proper lattice, which made the order we processed the worklist and called `LastStores::meet` observable: 1. We didn't have a single, canonical bottom value for the last store to a region. We were taking the first instruction in a block as an identifier for control-flow join points so that we would get different `MemoryLoc`s for different control-flow joins, which is necessary to avoid illegally forwarding a value loaded inside one control-flow join to a load in another, different control-flow join. However, this meant that we effectively had multiple bottom elements, which made the path we descended through the "lattice" observable. The solution here is to make `LastStore::Unknown` a proper bottom for the lattice and then add an "extent token" to `MemoryLoc`. The extent is computed incrementally as we push and pop blocks from a pre-order traversal of the dominator tree (which the egraphs pass that drives alias analysis already performs). 2. We computed the observed-stores set while we computed the fixpoint of the initial `LastStores` inputs to each block. This was incorrect, however, because a `LastStores` could transiently contain a `LastStore::Inst` that disappears in later iterations of the fixpoint, and which instructions do or don't transiently appear in `LastStores` in that way depends on the order in which we call `LastStores::meet`. Therefore, observing stores while computing the fixpoint might or might not observe an instruction depending on the worklist processing order. The solution in this case is to only compute the observed-stores set after we've computed the `LastStores` fixpoint, at which point there are no transient `LastStore::Inst`s anymore.
Instead of bitpacking the `AliasRegionKey` into a `u32`, hash it and then `xor`-fold the hash down to one byte. Fixes bytecodealliance#14221
ae2bf3c to
b6f8585
Compare
3 participants
Instead of bitpacking the
AliasRegionKeyinto au32, hash it and thenxor-fold the hash down to one byte.Fixes #14221
Depends on #14230