forked from WebKit/WebKit
-
Notifications
You must be signed in to change notification settings - Fork 51
wasm: poll VMTraps at loop back-edges so pure-Wasm loops can be terminated #372
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
robobun
wants to merge
1
commit into
main
Choose a base branch
from
farm/002e8c8f/wasm-loop-vm-traps
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
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
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
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
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
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
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
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
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
Oops, something went wrong.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 The slow-path patchpoint's effects leave
writesempty (onlyreads/exitsSidewaysare set), so B3 CSE forwards an outer loop's limit Load to an inner loop's — the inner back-edge then tests a value read once and never re-loaded, and a nested pure-Wasm loop in OMG never observesrequestStop(). Addeffects.writes = HeapRange::top();(matchingEffects::forCall(), which FTL'slazySlowPath()keeps by default).Extended reasoning...
What the bug is
In
OMGIRGenerator::addLoop(), the slow-path patchpoint's effects are built as:This leaves
effects.writesat its defaultHeapRange()(the empty range). Them_trapAwareSoftStackLimitload is a plainMemoryValue(no fence), so B3'seliminateCommonSubexpressionsis free to forward one loop's Load to another's whenever no block on any path between them has awritesset that overlapsHeapRange::top().The code path that triggers it
B3EliminateCommonSubexpressions.cpp:findMemoryValue()walks the CFG backward from a Load, bailing only ondata.writes.overlaps(range)(line 1028). It never consultsexitsSideways,fence(of other blocks), orreads. Per-blockdata.writesis populated at line ~349 viaif (HeapRange writes = effects.writes) clobber(...); an emptyHeapRangeis falsy, so the slow-path block contributes nothing and does not stop the walk.Both loads share the same
ptrkey:instanceValue()returns the single cachedm_instanceValue, and both areLoad pointerType()atoffsetOfTrapAwareSoftStackLimit(), so the Load-case filter (offset+opcode+type) matches.Why existing code doesn't prevent it
hoistLoopInvariantValuesbails on control-dependent loads when the loop has side exits), but that's a different pass — CSE has no such guard.exitsSidewayson the patchpoint prevents the patchpoint itself from being moved/eliminated, but CSE's predecessor walk for other loads only inspectsdata.writes.compileCheckTraps— which the comment cites as the model — avoids this becauselazySlowPath()creates aPatchpointValuewith the default constructor effects (B3PatchpointValue.cpp:54→Effects::forCall()), which haswrites = HeapRange::top(). That write barrier on the FTL slow-path block is exactly what makes CSE bail. The OMG code explicitly discards it.Step-by-step proof
Take
(loop $outer (loop $inner (br $inner))).addLoopproduces (per loop) body → Load, Above, Branch → {slowPath (Rare), continuation}; slowPath → patchpoint(writes=∅), Jump → continuation.br 0targets the Loop'sspecial= body, so the inner back-edge is inner_cont → inner_body.CSE processes Load₂ in inner_body:
m_data.writesis empty → walk predecessors {outer_cont, inner_cont}.memoryValuesAtTailcontains Load₁ (same ptr/offset/type) →matches = {Load₁}; continue.writes = ∅→ does not bail; preds already visited.memoryValuesAtTailcontains Load₂, but thematch != m_valueguard (line 1022) skips it; no writes → continue.writes = ∅→ does not bail.matches = {Load₁}(never reached the root).replaceMemoryValuesees a single match,RELEASE_ASSERT(outer_body dominates inner_body)passes (every path to inner_body goes through outer_body), and rewrites Load₂ → Identity(Load₁).The inner loop's
Branchnow testsAbove(Load₁, fp). The inner back-edge cycle inner_body ↔ inner_cont never re-executes outer_body, so Load₁ is read exactly once. A laterrequestStop()poisoningm_trapAwareSoftStackLimitis never observed and the OMG-compiled function spins forever — the exact hang this PR fixes, reintroduced for nested pure-Wasm loops (local.setetc. lower to B3 ops withwritesLocalStateonly, so realistic pure-compute inner loops qualify). OMG runs atoptLevel = 2(Options::wasmOMGOptimizationLevel()default), which enableseliminateCommonSubexpressionsinB3Generate.cpp, so this fires in the default configuration; the PR's single-loop repro wouldn't catch it.Fix
Add one line so CSE bails when it walks through the slow-path block via the back-edge:
(or equivalently start from
Effects::forCall()). This is confined to the Rare slow-path block, so the hot path still carries no clobber set as intended.