Skip to content

fix(gc): reload BOTH stale operands when one instruction has two (#7311 follow-up) - #7316

Merged
proggeramlug merged 1 commit into
mainfrom
fix/7311-multi-operand-reload
Aug 3, 2026
Merged

fix(gc): reload BOTH stale operands when one instruction has two (#7311 follow-up)#7316
proggeramlug merged 1 commit into
mainfrom
fix/7311-multi-operand-reload

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

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 r in rewrites {
    rename_operand(&mut insts[r.insn], &r.from, fresh);
    insts.insert(r.insn, reload);   // <-- shifts the consumer to r.insn + 1
}

For two rewrites on the same instruction, the first insert moves the consumer down by one. The second rename_operand then addresses insts[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.rs lowers object before value, 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:

%r7 = load double, ptr %r1
%r8 = load double, ptr %r2
%r6 = call double @js_object_assign_one(double %r7, double %r8)

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_reload tests pass with the fix; file-size, GC store-site and addr-class gates green; cargo fmt clean.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed an issue where instructions with multiple stale operands did not reload every operand correctly.
    • Ensured all affected operands are reloaded together, including cases involving object assignment.
    • Added regression coverage to verify correct operand replacement.

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a10a5741-f34f-482c-b2e7-c9ea772c8e52

📥 Commits

Reviewing files that changed from the base of the PR and between 2fc383a and 1688427.

📒 Files selected for processing (2)
  • changelog.d/7314-multi-operand-root-reload.md
  • crates/perry-codegen/src/root_reload.rs

📝 Walkthrough

Walkthrough

The 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.

Changes

Root reload correction

Layer / File(s) Summary
Group operand rewrites before reload insertion
crates/perry-codegen/src/root_reload.rs
Rewrites all stale operands against the original instruction, then inserts the generated reloads together.
Validate multi-operand reload behavior
crates/perry-codegen/src/root_reload.rs, changelog.d/7314-multi-operand-root-reload.md
Adds coverage for two stale shadow-slot operands and documents the affected js_object_assign_one case.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • PerryTS/perry#7116: Addresses stale GC-related operands during root reload through a related operand-reload path.
  • PerryTS/perry#7206: Addresses the same stale-register/root-reload problem in different functions.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/7311-multi-operand-reload

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug
proggeramlug merged commit 6bedb25 into main Aug 3, 2026
28 of 41 checks passed
@proggeramlug
proggeramlug deleted the fix/7311-multi-operand-reload branch August 3, 2026 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants