fix(gc): reload BOTH stale operands when one instruction has two (#7311 follow-up) - #7316
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe root reload pass now groups rewrites per instruction before inserting loads. A regression test covers an instruction with two stale shadow-slot operands, and the changelog records the fix. ChangesRoot reload correction
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
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
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.
Follow-up to #7311, addressing a Major review comment that was correct.
The defect
root_reload's apply loop renamed an operand and then inserted its reload, one rewrite at a time:For two rewrites on the same instruction, the first
insertmoves the consumer down by one. The secondrename_operandthen addressesinsts[r.insn]— which is now the reload just inserted, not the consumer. It matches nothing, so the second operand stays stale and a load nothing consumes is emitted.Why it matters more than a dead load
js_object_assign_one(receiver, value)is exactly that shape —index_set.rslowersobjectbeforevalue, so both operands can be loads out of their own shadow slots — and it is the population #7311 reports going 137 → 0 on the dependency-scale corpus.A per-instruction violation count can reach zero while one operand is still stale. So the headline number was measuring less than it appeared to, in precisely the case the pass exists to fix.
The fix
Group rewrites by
(block, instruction): rename every operand against the original instruction, then insert that instruction's reloads. Order among the reloads is immaterial — each defines a distinct register and they are independent loads.Verification
New
both_stale_operands_of_one_instruction_are_reloaded, built on the module's existing fixture idiom, asserting the emitted IR:It checks both reloads exist, that the consumer reads both, and — deriving the register names rather than hard-coding them — that neither pre-call load survives in the consumer.
Sabotage-checked: restoring the one-at-a-time loop fails the test. 12/12
root_reloadtests pass with the fix; file-size, GC store-site and addr-class gates green;cargo fmtclean.Summary by CodeRabbit