Move some free standing functions to ir::InstructionInfo methods - #8209
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR moves instruction sizing, location, predicate, and rewrite helpers onto ChangesInstructionInfo method migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
🧹 Nitpick comments (1)
crates/codegen/src/ir.rs (1)
2682-2713: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the three super-instruction arms into one call.
All three match arms differ only in the
super_opvalue while sharing thesplit_at_mut(1)+make_super_instruction(...)logic. Extract the differing opcode first, then call once. The first arm's nestedifis equivalent to matching(Some(Opcode::LoadFast), Some(Opcode::LoadFast))directly.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."
♻️ Proposed refactor
- match (block.instructions[i].instr.real_opcode(), nextop) { - (Some(Opcode::LoadFast), _) => { - if matches!(nextop, Some(Opcode::LoadFast)) { - let (inst1, rest) = block.instructions[i..].split_at_mut(1); - InstructionInfo::make_super_instruction( - &mut inst1[0], - &mut rest[0], - Opcode::LoadFastLoadFast.into(), - ); - } - } - - (Some(Opcode::StoreFast), Some(Opcode::LoadFast)) => { - let (inst1, rest) = block.instructions[i..].split_at_mut(1); - InstructionInfo::make_super_instruction( - &mut inst1[0], - &mut rest[0], - Opcode::StoreFastLoadFast.into(), - ); - } - - (Some(Opcode::StoreFast), Some(Opcode::StoreFast)) => { - let (inst1, rest) = block.instructions[i..].split_at_mut(1); - InstructionInfo::make_super_instruction( - &mut inst1[0], - &mut rest[0], - Opcode::StoreFastStoreFast.into(), - ); - } - - (_, _) => {} - } + let super_op = match (block.instructions[i].instr.real_opcode(), nextop) { + (Some(Opcode::LoadFast), Some(Opcode::LoadFast)) => { + Some(Opcode::LoadFastLoadFast) + } + (Some(Opcode::StoreFast), Some(Opcode::LoadFast)) => { + Some(Opcode::StoreFastLoadFast) + } + (Some(Opcode::StoreFast), Some(Opcode::StoreFast)) => { + Some(Opcode::StoreFastStoreFast) + } + (_, _) => None, + }; + if let Some(super_op) = super_op { + let (inst1, rest) = block.instructions[i..].split_at_mut(1); + InstructionInfo::make_super_instruction( + &mut inst1[0], + &mut rest[0], + super_op.into(), + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/codegen/src/ir.rs` around lines 2682 - 2713, Refactor the match in ir.rs that builds super-instructions so the repeated split_at_mut(1) and InstructionInfo::make_super_instruction call happens only once. In the match around block.instructions[i].instr.real_opcode(), first determine the super_op for the three supported pairs (LoadFast/LoadFast, StoreFast/LoadFast, StoreFast/StoreFast), then perform the shared mutation once using that opcode; the current nested if in the LoadFast arm should be treated as a direct (Some(Opcode::LoadFast), Some(Opcode::LoadFast)) case.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/codegen/src/ir.rs`:
- Around line 2682-2713: Refactor the match in ir.rs that builds
super-instructions so the repeated split_at_mut(1) and
InstructionInfo::make_super_instruction call happens only once. In the match
around block.instructions[i].instr.real_opcode(), first determine the super_op
for the three supported pairs (LoadFast/LoadFast, StoreFast/LoadFast,
StoreFast/StoreFast), then perform the shared mutation once using that opcode;
the current nested if in the LoadFast arm should be treated as a direct
(Some(Opcode::LoadFast), Some(Opcode::LoadFast)) case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: eb7380ae-bd2f-41ca-9d7c-3c0fce2b6daa
📒 Files selected for processing (1)
crates/codegen/src/ir.rs
Summary
Summary by CodeRabbit