Make LastStores a proper lattice - #14230
Merged
Merged
Conversation
github-actions
Bot
added
the
cranelift
fitzgen
force-pushed
the
alias-analysis-lattice
branch
from
August 31, 2026 14:48
4ad30ed to
6e847e9
Compare
cfallin
reviewed
Aug 31, 2026
cfallin
left a comment
cfallin
left a comment
Member
There was a problem hiding this comment.
Thanks for thinking through this carefully -- definitely very subtle.
Some comments as I read below, but more importantly I have a high-level feeling of "this may be getting too complex", and I'm wondering how much of it we really need for our stated purposes. In particular, what I am seeing is that there are a whole bunch of specific complexities coming out of the one core decision to support inter-block merged versions better than "unknown":
- The distinction between
MemoryVersionandLastStore, and conversion between them; - The pre-pass to find "merge blocks", and the subtle semantics around the lattice value that refers to this (external) table. In particular I'm having trouble thinking about "merge token"-version values that flow across other merge blocks (e.g. within a subregion) -- why is this correct?
- The observe-pass thing with
Option<HashMap>and confusing control flow.
I wonder if we could benchmark the alternative where we fix the lattice the other way -- no loc-keyed new version that occurs at a meet-point, just send that to bottom instead. That's a much smaller fix that results in an enormously simpler analysis, and I think it would get at least the straight-line trampoline cases we immediately care about, and many of the intra-block and even simple inter-block opts for e.g. GC fields with separate regions. The only case it misses is where we have a merge point, no store (to do a strong-update and overwrite the identity) but instead a load, and we can't RLE a second load. But actually even in that case we could cache the loaded result on a key with the load's instruction, no? So I'm not seeing where we would actually lose anything with the simpler analysis. Happy to see counterexamples or hear your reasoning on this of course!
Member
Author
It is not that we want to support better merged versions than "unknown", it is that for correctness we need to distinguish between these different control-flow join points so that we get different
Again, I believe this is inherent complexity, and is best dealt with by being honest about that and modeling it in the types:
Trying to have one thing satisfy two conflicting requirements simultaneously is going to lead to bugs in practice (as we have already seen, by realizing that the worklist traversal order is observable).
I'm not sure I follow the question. Merge blocks do not flow across other merge points. A block's merge block is the first predecessor (or itself) that is either (a) the function entry or (b) has
We have some options here, and I don't particularly love any of them. We could do any of:
We are balancing code duplication, complexity, and whether or not we skip unnecessary work. I don't know how we can optimize all at once, though.
As mentioned above, that would be unsound, because it means that we could have:
If we process block A first, then we will insert MemoryLoc {
last_store: None, // or whatever bottom is represented as
address,
offset,
ty,
extending_opcode,
endianness,
}as the key for the known value that we just loaded. Then, when we process block B, it will look up the exact same Zooming out, the only other way to avoid the merge blocks, without salting our |
Member
Yes, agreed that it was a pre-existing issue. There is something I still don't understand though. It seems that the above is assuming that we would still associate known values with locs that have "bottom" locations (i.e., merged locations). I was assuming (and I think had implied?) that we would simply not associate values with such locations. That's far simpler and avoids all of this machinery, no? |
Member
Author
Ah, I see what you're suggesting now, I had misunderstood your comment. I prototyped this, not adding entries to DetailsA lot of those look like regressions to pretty simple cases that I'd really expect to be handled. We also get the following DetailsSo it looks like we are missing optimizations (mostly for GC- and component model adapter-related stuff) in practice, not just in theory for our filetests that attempt to enumerate all the cases. |
Member
Author
|
The more I think about it, the more the scoped hash map I mentioned above becomes more appealing, since "scope" and where it is valid to reuse a known memory value is really what we are trying to capture here... |
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.
fitzgen
force-pushed
the
alias-analysis-lattice
branch
from
September 3, 2026 21:14
6e847e9 to
610904d
Compare
Member
Author
|
Just pushed a commit with the scope API version of the fix discussed above. I like this much better: we can incrementally compute extents (merge tokens) in a very simple, straightforward way and we can get rid of the post-pass to compute each block's extent and we can also get rid of the |
cfallin
approved these changes
Sep 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There were two ways in which alias analysis's
LastStoresstate was not a proper lattice, which made the order we processed the worklist and calledLastStores::meetobservable: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
MemoryLocs 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 was to create a separateLastStoredataflow value that has a single, canonical bottom element, and a distinctMemoryVersionvalue that is the same asLastStorebut replaces its bottom value with a variant that identifies the associated control-flow join point. We useLastStorein ourLastStoreslattice, when we need a bottom element, and we useMemoryVersionin ourMemoryLockeys, to distinguish between different regions where we don't know anything about the contents of memory.We computed the observed-stores set while we computed the fixpoint of the initial
LastStoresinputs to each block. This was incorrect, however, because aLastStorescould transiently contain aLastStore::Instthat disappears in later iterations of the fixpoint, and which instructions do or don't transiently appear inLastStoresin that way depends on the order in which we callLastStores::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 was to only compute the observed-stores set after we've computed theLastStoresfixpoint, at which point there are no transientLastStore::Insts anymore.