AnyInstruction helpers + doc imrovments#7757
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors bytecode/IR handling to use opcode-based APIs ( Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/codegen/src/ir.rs`:
- Around line 4291-4296: The match arm for Opcode::LoadFastBorrowLoadFastBorrow
incorrectly uses a destructuring pattern with `{ .. }` on a unit variant; update
the pattern in the match over info.instr.real_opcode() to use the unit variant
form Some(Opcode::LoadFastBorrowLoadFastBorrow) => { ... } (leaving the body
that sets info.instr = Opcode::LoadFastLoadFast.into() unchanged) so it compiles
like the other Opcode matches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 9f981275-9e79-44e4-89fd-a67efd45c11d
📒 Files selected for processing (1)
crates/codegen/src/ir.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/codegen/src/ir.rs (1)
3074-3080: ⚡ Quick winFactor out the repeated borrow-deopt remap.
The same
LoadFastBorrow* -> LoadFast*rewrite is duplicated across several deopt passes. A small shared helper would make this opcode migration much harder to drift the next time borrow variants change.♻️ Suggested cleanup
+fn deoptimize_borrowed_fast(info: &mut InstructionInfo) -> bool { + match info.instr.real_opcode() { + Some(Opcode::LoadFastBorrow) => { + info.instr = Opcode::LoadFast.into(); + } + Some(Opcode::LoadFastBorrowLoadFastBorrow) => { + info.instr = Opcode::LoadFastLoadFast.into(); + } + _ => return false, + } + true +} + fn deoptimize_block_borrows(block: &mut Block) { for info in &mut block.instructions { - match info.instr.real_opcode() { - Some(Opcode::LoadFastBorrow) => { - info.instr = Opcode::LoadFast.into(); - } - Some(Opcode::LoadFastBorrowLoadFastBorrow) => { - info.instr = Opcode::LoadFastLoadFast.into(); - } - _ => {} - } + let _ = deoptimize_borrowed_fast(info); } }As per coding guidelines, "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code".
Also applies to: 3254-3260, 3515-3521, 3669-3675, 3722-3727, 3829-3835, 4246-4253, 4569-4577
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/ir.rs` around lines 3074 - 3080, Extract a small helper (e.g. fn remap_borrow_variants(op: Opcode) -> Option<Opcode>) that maps borrow forms to their non-borrow equivalents (handle Opcode::LoadFastBorrow -> Opcode::LoadFast and Opcode::LoadFastBorrowLoadFastBorrow -> Opcode::LoadFastLoadFast, etc.), then replace the duplicated match blocks by calling this helper: call info.instr.real_opcode(), pass it into remap_borrow_variants, and if Some(mapped) set info.instr = mapped.into(); apply the same helper usage at the other duplicated locations that reference Opcode::LoadFastBorrow, Opcode::LoadFastBorrowLoadFastBorrow, Opcode::LoadFastLoadFast and info.instr to avoid drifting copies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/codegen/src/ir.rs`:
- Around line 3074-3080: Extract a small helper (e.g. fn
remap_borrow_variants(op: Opcode) -> Option<Opcode>) that maps borrow forms to
their non-borrow equivalents (handle Opcode::LoadFastBorrow -> Opcode::LoadFast
and Opcode::LoadFastBorrowLoadFastBorrow -> Opcode::LoadFastLoadFast, etc.),
then replace the duplicated match blocks by calling this helper: call
info.instr.real_opcode(), pass it into remap_borrow_variants, and if
Some(mapped) set info.instr = mapped.into(); apply the same helper usage at the
other duplicated locations that reference Opcode::LoadFastBorrow,
Opcode::LoadFastBorrowLoadFastBorrow, Opcode::LoadFastLoadFast and info.instr to
avoid drifting copies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: fe5d138f-eb5b-4a8a-b53b-e6e107c5fdc0
📒 Files selected for processing (1)
crates/codegen/src/ir.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/codegen/src/ir.rs (1)
3074-3081: ⚡ Quick winExtract shared borrow-deopt rewrite into one helper
The same
LoadFastBorrow* -> LoadFast*rewrite logic is duplicated across multiple passes. Please centralize this into a small helper to avoid future drift.Proposed refactor sketch
+fn deoptimize_borrow_instr(info: &mut InstructionInfo) { + info.instr = match info.instr.real_opcode() { + Some(Opcode::LoadFastBorrow) => Opcode::LoadFast, + Some(Opcode::LoadFastBorrowLoadFastBorrow) => Opcode::LoadFastLoadFast, + _ => return, + } + .into(); +} ... - for info in &mut block.instructions { - match info.instr.real_opcode() { - Some(Opcode::LoadFastBorrow) => { - info.instr = Opcode::LoadFast.into(); - } - Some(Opcode::LoadFastBorrowLoadFastBorrow) => { - info.instr = Opcode::LoadFastLoadFast.into(); - } - _ => {} - } - } + for info in &mut block.instructions { + deoptimize_borrow_instr(info); + }As per coding guidelines: "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code".
Also applies to: 3254-3261, 3515-3522, 3669-3676
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/ir.rs` around lines 3074 - 3081, Extract the repeated "borrow-deopt" rewrite into a small helper (e.g., rewrite_loadfast_borrow) and call it from each pass instead of duplicating the match; the helper should take a mutable reference to the instruction container used in these passes (the same type as `info`), pattern-match `info.instr.real_opcode()` and map `Opcode::LoadFastBorrow` -> `Opcode::LoadFast` and `Opcode::LoadFastBorrowLoadFastBorrow` -> `Opcode::LoadFastLoadFast`, updating `info.instr` accordingly. Replace the duplicated match blocks currently in the functions that contain the shown snippet and the similar blocks at the other locations (around the code handling 3254-3261, 3515-3522, 3669-3676) with a single call to this helper to centralize the logic and avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/codegen/src/ir.rs`:
- Around line 3074-3081: Extract the repeated "borrow-deopt" rewrite into a
small helper (e.g., rewrite_loadfast_borrow) and call it from each pass instead
of duplicating the match; the helper should take a mutable reference to the
instruction container used in these passes (the same type as `info`),
pattern-match `info.instr.real_opcode()` and map `Opcode::LoadFastBorrow` ->
`Opcode::LoadFast` and `Opcode::LoadFastBorrowLoadFastBorrow` ->
`Opcode::LoadFastLoadFast`, updating `info.instr` accordingly. Replace the
duplicated match blocks currently in the functions that contain the shown
snippet and the similar blocks at the other locations (around the code handling
3254-3261, 3515-3522, 3669-3676) with a single call to this helper to centralize
the logic and avoid drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8be3551c-0ae3-47fd-9d76-a8db7fa12480
📒 Files selected for processing (1)
crates/codegen/src/ir.rs
Summary by CodeRabbit