Repository navigation
B3: follow-up to #982: a Phi that follows its own Upsilon, and a check for stale Value::owner - #986
Jarred-Sumner wants to merge 3 commits into
Conversation
…arried
The loop phases (loop versioning, loop invariance, range check removal, unrolling) assume that a Phi in a loop header is loop-carried: its
Upsilon outside the loop supplies the value on entry, and the Upsilons inside the loop supply the value for the next iteration.
That does not hold for a Phi left over from an `if` that has lost one of its arms:
BB#1: ; ends in Oops, on no path into the loop
Upsilon($3, ^phi)
Oops
BB#2: ; the loop header
Upsilon(@x, ^phi)
Check(...)
@phi = Phi()
@index = BitAnd(Add(@phi, @i), $15)
Branch(BelowEqual(@index, @limit)) ; a range check that leaves the loop
@phi is @x on every iteration. Loop invariance took it for 3 on the first iteration, and loop versioning copied the read of the Phi's
variable into the guard block before the fast loop, where nothing has set it. fixSSA fills in a zero for such a read, so the guard that
decides whether the loop may run without its range checks was computed from the wrong value. Since #982 fixSSA reports that as an internal
compiler error (a variable is read before it is set), which is how this was found.
eliminateRedundantPhiCycles, which already runs before the loop phases and between them, now begins by replacing such a Phi with an Identity
of the Upsilon's child and turning the Phi's Upsilons into Nops. A Phi reads the value of the most recently executed Upsilon, and an Upsilon
that precedes it in the same block is always that one. Its child dominates the Upsilon, so it dominates every use of the Phi.
The JIT does not run this phase: every pipeline step that calls it requires findLoops to have run, which needs one of the loop policies,
and they are all off by default.
Tests, in testb3: a hand-written graph on which loop versioning alone ends in "read before it is set", and which gives the right results
when the preceding phase runs first; eliminateRedundantPhiCycles on such a Phi with and without a second Upsilon in another block. The
latter replaces a test that pinned the old behavior (the Phi was kept).
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Fold Phis and validate loop versioning Source/JavaScriptCore/b3/B3EliminateRedundantPhiCycles.cpp, Source/JavaScriptCore/b3/testb3_bun_loops.cpp, Source/JavaScriptCore/b3/testb3_bun_version_loops.cpp |
The phase folds eligible Phis to the child of a preceding matching Upsilon, then runs cycle elimination. Tests cover same-block folding and loop versioning with and without the phase. |
Assertion-Only Stale-Owner Checks
| Layer / File(s) | Summary |
|---|---|
Check value owners before generation steps and cleanups Source/JavaScriptCore/b3/B3Generate.cpp |
Assertion-enabled builds poison mismatched value owners before enabled steps and eligible cleanups, unless the procedure has a reported failure. |
Possibly related PRs
- oven-sh/WebKit#906: Changes the cycle-elimination implementation that this PR now runs after Phi folding.
- oven-sh/WebKit#979: Changes loop-header Phi classification and adds a related loop identity regression test.
Merge Risk: 
18823
-
No merge-blocking issue is established by the available evidence.
Pre-merge checks: 
-
Check name Status Explanation Title check The title names two changes in B3: a fix for a Phi that follows its own Upsilon, and a check for stale Value::owner. The raw summary and objectives confirm both. The title is specific and matches the … Description check The description follows the WebKit-style structure closely enough for a PR. It has a clear explanation of the two fixes, the scope of impact on release builds, and the tests added. It links the follow… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names two changes in B3: a fix for a Phi that follows its own Upsilon, and a check for stale Value::owner. The raw summary and objectives confirm both. The title is specific and matches the main change. It is long at 93 characters, and it names two changes, but it still summarizes the primary change clearly.
Full details: Description check
-
Explanation
The description follows the WebKit-style structure closely enough for a PR. It has a clear explanation of the two fixes, the scope of impact on release builds, and the tests added. It links the follow-up to
#982and states the test results. It does not include the bug title and Bugzilla link, the 'Reviewed by' line, or the changed-files list in the template format. These are required by the template, and the template says manually-filed PRs should use the commit message as the description. The author lists the files by path only in prose, so the required changelog-style file list is missing.Resolution
Consider adding the Bugzilla bug title and link, which the template requires, and the per-file ChangeLog-style entries (path plus function names). The 'Reviewed by' line can be left for reviewers.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
The commit message of the fix says what they said. No code changes.
…date Value::owner is valid only after Procedure::resetValueOwners(). A value that an InsertionSet added has a null owner, and a value that was moved to another block keeps its old one. A later phase that reads owners without resetting them first silently works with the wrong block, or gets away with it because an earlier phase happened to reset them. promoteLoopMemory did that: it was entered with null owners in 90 procedures of the TypeScript compiler, and nothing reported it. In builds with assertions, the pipeline's driver now overwrites every owner that is not the block that holds the value with an unmapped address, before each step and each cleanup. A phase that calls resetValueOwners() first does not notice. One that does not crashes on the first procedure it sees. A release build compiles none of this. It is on for every client of the pipeline, the JIT's tiers included: with assertions and ASAN all of testb3 and testair pass, and JSTests/stress in every FTL mode fails the same seven tests with it as without it.
Jarred-Sumner
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes what a B3 optimization pass does to Phis and adds an assertion-build hook that runs before every pipeline step, a human look is still worthwhile.
What was reviewed:
- The fold in
B3EliminateRedundantPhiCycles.cpp: the Upsilon picked is the last same-block one before the Phi, so it is always the most recent store at the Phi; its child dominates the Phi, the other Upsilons to that Phi are dead and are Nop'd, chained Identities resolve throughfoldIdentity, and neither is visible to thePhiChildrenpass that follows. - The poison hook in
B3Generate.cpp: it is inside the file's anonymous namespace, runs after the frequency/edge-class updates, and each default-pipeline step that readsValue::owner(moveConstants, reduceStrength/pureCSE, CSE, threadJumps, sinkValues, lowerToAir) either callsresetValueOwners()or writes owner itself before reading it. - Ruled out: a folded Phi still fed by a live Upsilon elsewhere (a Phi reads the most recently executed Upsilon, so those are dead), and the fold only running once (it also runs after versionLoops and after the second rotateLoops).
Extended reasoning...
The change touches four files under Source/JavaScriptCore/b3: a new pre-pass in eliminateRedundantPhiCycles that replaces a Phi with the child of its same-block preceding Upsilon, a debug-only owner-poisoning helper called before every step and cleanup in the B3 pipeline, and two testb3 tests (one replacing a test that pinned the old behavior). It touches no auth, crypto, or input-handling surface; the release-build effect is confined to loop policies that are off by default. Not approving because this is semantic compiler-pass logic rather than a mechanical change, the paths fall under the Source/JavaScriptCore CODEOWNERS entry, and the assertion-build hook will only be exercised locally or in assertion fuzzing lanes rather than by the tested CI lanes.
|
Preview build of 1882392: |
Follow-up to #982: one fix in B3's loop phases and one check for assertion builds, a commit each. The first commit message has the B3 program that broke and what the compiler did with it.
Every file is under
Source/JavaScriptCore/b3.What it fixes
B3EliminateRedundantPhiCycles.cpp). Loop versioning computed the guard of a loop without range checks from a variable that nothing had set. Since AOT: 77 fixes: crashes and wrong answers in executables, mocks and import() in test runs #982 that is an internal compiler error instead of wrong code; now the graph compiles, and correctly.Value::ownerpointers that are not up to date (B3Generate.cpp), so a phase that reads owners without callingresetValueOwners()crashes at once instead of reading a stale or null block.What changes for plain Bun
Nothing in a release build.
eliminateRedundantPhiCycles: the JIT's pipeline never runs it. Each step that calls it requiresfindLoops, which runs only under a loop policy (rotates,versions,vectorizes,unrolls,promotesMemory,foldsComparisonsWithBounds), and those are all off in the default policy.#if ASSERT_ENABLED. In debug and ASAN builds it runs before each phase of the B3 pipeline, for the JIT too.Tests
testb3_bun_version_loops.cpp:testVersionLoopsPhiThatFollowsItsUpsilonInHeaderIsNotLoopCarried.testb3_bun_loops.cpp:testEliminateRedundantPhiCyclesFoldsPhiThatFollowsItsUpsilon, in place of a test that pinned the old behavior. All oftestb3passes at all three optimization levels.