Access component-model flags via their VmType alias region in adapters - #14116
Open
fitzgen wants to merge 7 commits into
Open
Access component-model flags via their VmType alias region in adapters#14116fitzgen wants to merge 7 commits into
VmType alias region in adapters#14116fitzgen wants to merge 7 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.
These flags were imported by adapters and accessed with the `PublicGlobal` alias
region, but this led to ambiguity between loads and stores to different
flags. The flags are stored in the `VMComponentContext` and we generally know
when we are importing flags as globals unambiguously, so do the same thing we do
to get precise alias regions for defined globals for component-model flags, but
use the `AliasRegionKey::Vm { ty: VMComponentContext, .. }` alias region.
A final wrinkle: core Wasm does not have the enclosing `VMComponentContext`'s
offsets on hand, so we move `task_may_block` ahead of `may_leave` so both flags
are before every component-shape-dependent field and the flags' offsets are a
function of the pointer size alone.
github-actions
Bot
added
cranelift
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
1 participant
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.
These flags were imported by adapters and accessed with the
PublicGlobalaliasregion, but this led to ambiguity between loads and stores to different
flags. The flags are stored in the
VMComponentContextand we generally knowwhen we are importing flags as globals unambiguously, so do the same thing we do
to get precise alias regions for defined globals for component-model flags, but
use the
AliasRegionKey::Vm { ty: VMComponentContext, .. }alias region.A final wrinkle: core Wasm does not have the enclosing
VMComponentContext'soffsets on hand, so we move
task_may_blockahead ofmay_leaveso both flagsare before every component-shape-dependent field and the flags' offsets are a
function of the pointer size alone.
Depends on #14115