GeezerVison

crates/codegen/src/ir.rs (2)

1164-1210: Consider extracting the repeated has_fallthrough predicate.

The same three-line pattern:

.last().map(|ins| !ins.instr.is_scope_exit() && !ins.instr.is_unconditional_jump()).unwrap_or(true)

appears in mark_cold (line 1183–1185), push_cold_blocks_to_end (line 1231–1234), and label_exception_targets. A small free function would eliminate the duplication.

♻️ Proposed helper

+/// Returns true if `block` falls through to its `next` block
+/// (i.e. its last instruction is neither a scope exit nor an unconditional jump).
+fn block_has_fallthrough(block: &Block) -> bool {
+    block
+        .instructions
+        .last()
+        .map(|ins| !ins.instr.is_scope_exit() && !ins.instr.is_unconditional_jump())
+        .unwrap_or(true)
+}

🤖 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 1164 - 1210, The three-line
fallthrough predicate (.instructions.last().map(|ins| !ins.instr.is_scope_exit()
&& !ins.instr.is_unconditional_jump()).unwrap_or(true)) is duplicated across
mark_cold, push_cold_blocks_to_end, and label_exception_targets; extract it as a
small free function (e.g., fn last_allows_fallthrough(block: &Block) -> bool or
fn instr_allows_fallthrough(ins: Option<&InstrWrapper>) -> bool) and replace the
inline expression in those functions with a call to the new helper, keeping
semantics identical (true when no last instruction, otherwise !is_scope_exit &&
!is_unconditional_jump).

---

1252-1271: Document the cold→warm jump invariant at the insertion site.

PseudoInstruction::JumpNoInterrupt is correctly included in is_unconditional_jump() for both the fixup filter and normalize_jumps, so no functional issue exists there.

However, the ordering invariant that enables the unconditional conversion to JumpBackwardNoInterrupt in finalize_code (line 318) is implicit: JumpNoInterrupt is inserted here before cold blocks are reordered to the end, and only after reordering does every cold→warm jump become backward. This dependency should be documented at the insertion site (around line 1257) to clarify why the later conversion is safe, and optionally reinforced with a debug_assert! at conversion time.

🤖 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 1252 - 1271, Document that the
JumpNoInterrupt insertion relies on the invariant that cold blocks are moved to
the end later so that any cold→warm JumpNoInterrupt becomes a backward jump
during finalize_code; add a brief comment at the jump insertion site (around
jump_block creation / PseudoInstruction::JumpNoInterrupt) explaining this
dependency and referencing normalize_jumps and finalize_code, and add a
debug_assert! in finalize_code near the conversion to JumpBackwardNoInterrupt
that checks the jump target is backward for JumpNoInterrupt (so the invariant is
validated in debug builds).

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro