Use precise alias regions for statically-known entity imports - #14115
Open
fitzgen wants to merge 6 commits into
Open
Use precise alias regions for statically-known entity imports#14115fitzgen wants to merge 6 commits into
fitzgen wants to merge 6 commits into
Conversation
Alias analysis's dead-store elimination removed the dead store's `mem_values`
entry, but left the region's last-store slot naming the instruction it had just
deleted. Leaving the removed-store meant that when we then reprocess the
overwriting store, we keyed its lookup on a removed instruction, found nothing,
and failed to notice that (for example) the overwriting store became idempotent
and could also be removed.
With this commit, each store now records the memory version it displaced, and
eliminating a dead store rolls that version back, so a chain like
v1 = load.i32 region0 v0
store region0 v2, v0 ;; dead
store region0 v1, v0 ;; idempotent once the dead store is gone
collapses in the single pass we actually make, rather than removing only one
link in the chain and requiring that we do N passes to fully clean up a chain of
N dead/idempotent stores. This code pattern the shape fused sync adapters emit
around the `MAY_LEAVE` flag and the relevant disas tests each lose a store as a
result.
Before, we would emit a call to a host function, passing the trap code as a constant argument. Now we emit a `trap <code>` instruction directly. This is a large improvement for our sync adapter disas tests.
If a load is marked `notrap` then it is not side-effecting and we need not emit it when its loaded value is unused. Note that we *do* still have to increment the side effect color for these instructions to prevent merging/sinking loads across stores that could change the value they observe. This drops two dead vmctx flag loads from our component-model fused adapter's fast path.
Accesses of statically-known globals, memories, and tables now use
`AliasRegionKey::Defined{Global,Memory,Table}` rather than the conservative
`Public{Global,Memory,Table}` region shared by every entity of that kind which
crosses a module boundary.
However, unlike `known_imported_functions`, this requires an extra condition:
every module that ever imports an entity must always import that same
entity. Otherwise a function that accesses, e.g., a memory via the conservative
region could be inlined into one that uses the precise region, and accessing the
same bytes through two different alias regions is invalid and leads to
miscompiles. That is, all of the importing modules and the defining module must
agree on the alias region.
github-actions
Bot
added
cranelift
alexcrichton
left a comment
alexcrichton
left a comment
Member
There was a problem hiding this comment.
Thinking about this analysis done here I'm worried about the case where a module imports something and reexports it, although I can't quite place my finger on why so I wanted to ask about that. It seems like an import could be considered unambiguous but then an export could be considered ambiguous, but in that scenario we'd want the import to additionally be considered ambiguous. Does this handle that sort of case already?
| // statically knows that its import is always this memory, so we can | ||
| // only use this memory's precise region when every module that may | ||
| // import it does know that. Memory accessed with two different alias | ||
| // regions must not actually alias, or else we will get miscompiles. |
Member
There was a problem hiding this comment.
Could the comment here, or somewhere in this function, indicate that the global/table version of this function below is a copy/paste of this function and they should all stay in sync?
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.
Accesses of statically-known globals, memories, and tables now use
AliasRegionKey::Defined{Global,Memory,Table}rather than the conservativePublic{Global,Memory,Table}region shared by every entity of that kind whichcrosses a module boundary.
However, unlike
known_imported_functions, this requires an extra condition:every module that ever imports an entity must always import that same
entity. Otherwise a function that accesses, e.g., a memory via the conservative
region could be inlined into one that uses the precise region, and accessing the
same bytes through two different alias regions is invalid and leads to
miscompiles. That is, all of the importing modules and the defining module must
agree on the alias region.
Depends on #14114