Virtual stack

[navicore]

No description provided.

[claude]

test

[claude]

PLACEHOLDER - will replace

[claude]

Code Review: Virtual Stack (PR 360)

This is a solid refactoring PR that extracts repeated LLVM IR generation patterns into well-named helpers. The net -165 lines while adding layout-aware helpers is a meaningful improvement.

Positives: Consistent emit_* naming convention. Good doc comments explaining 40-byte vs tagged-ptr semantics. Centralizing layout-dependent logic in layout.rs. Substantial boilerplate reduction in ops.rs and control_flow.rs.

Issue 1 - emit_load_two_float_operands inconsistent return value: The function returns (ptr_a, slot1_a, val_a, val_b) but the semantics of the second element differ by mode: in 40-byte mode it is slot1_a (different from ptr_a); in tagged-ptr mode it is just ptr_a.clone() (redundant). One call site already ignores it with _slot1_a. Consider a named struct.

Issue 2 - Unimplemented tagged-ptr float handling silently generates incorrect code: Both emit_load_two_float_operands and emit_store_float_bits have TODO branches in tagged-ptr mode that fall back to raw bit operations. These will silently produce incorrect IR if tagged-ptr mode is used with floats. Prefer todo!() / unimplemented!() to make failures loud rather than silent.

Issue 3 - Weakened test assertion in mod.rs: Before: assert!(ir.contains("trunc i64 %result to i32")); After: two separate contains checks for "trunc i64 %" and "to i32". These can now pass even if the two strings appear in unrelated instructions. A line-level check would be stronger: assert!(ir.lines().any(|l| l.contains("trunc i64 %") && l.contains("to i32")));

Issue 4 - discriminant() suppressed with allow(dead_code) rather than removed: The spill code no longer calls it. Either remove it or add a comment explaining which future path needs it.

Minor Nits: emit_store_float_result parameter slot1_or_ptr leaks layout internals to callers; store_target would be more neutral. No new unit tests for emit_* helpers in layout.rs.

Summary: Good refactor overall. Main actionable items: (1) Use todo!() for tagged-ptr float TODOs instead of silently wrong fallbacks. (2) Strengthen the weakened test assertion in mod.rs. (3) Remove or properly document discriminant().

[claude]

PR Review: Virtual Stack (#360)

This is a solid refactoring that significantly reduces code duplication by centralizing layout-specific LLVM IR generation into helper methods in layout.rs. Net -127 lines with improved clarity is a win.

---

What's Good

• DRY improvement: Replacing repeated GEP+load sequences with emit_stack_gep, emit_load_int_payload, emit_load_two_int_operands, etc. is exactly right — the old code had the same 10-15 line patterns copy-pasted across 5+ callsites.
• Layout abstraction: Centralizing 40-byte vs tagged-ptr branching in layout.rs is the correct architectural choice. Callers no longer need to know internal slot offsets.
• Tests added: The new layout.rs tests verify both modes and explicitly document the expected IR patterns. The panic tests for unimplemented tagged-ptr float paths are a nice touch.
• Test loosening in mod.rs: Changing the assertion from ir.contains("trunc i64 %result to i32") to .any(|l| l.contains("trunc i64 %") && l.contains("to i32")) is correct — the SSA variable name is now dynamic, so the old test was fragile.

---

Issues

1. Duplicate doc comment on emit_store_float_result (layout.rs ~line 657)

The doc comment has two opening lines that describe the same thing — looks like an old draft wasn't fully removed:

/// Store a float result (as double) at the given stack pointer.
/// In 40-byte mode: stores bits to slot1 (discriminant 1 already at slot0).
/// Store a float result (as double) at the given store target.   // <-- leftover

The first two lines should be dropped or merged with the third.

2. todo!() panics instead of errors for tagged-ptr float paths

emit_load_two_float_operands, emit_store_float_result, and emit_store_float_bits all todo!() in tagged-ptr mode. Since CodeGenError is already the error type, these should return Err(CodeGenError::Unimplemented(...)) (or similar) rather than panic. A todo!() in a code path the compiler might hit is a UX cliff — the user gets a panic with no context instead of a compile error.

// Prefer this pattern:
return Err(CodeGenError::Unimplemented("tagged-ptr float load".into()));
// Over:
todo!("tagged-ptr float load not yet implemented")

(The existing should_panic tests would need to change to Err(...) checks, but that's better coverage anyway.)

3. Leaky abstraction: slot1_a in emit_load_two_float_operands return

The function returns (ptr_a, slot1_a, val_a, val_b) where slot1_a is the getelementptr i64 into slot1 — an internal 40-byte layout detail exposed to callers. In codegen_inline_float_comparison, this shows up as:

let (ptr_a, _slot1_a, val_a, val_b) = self.emit_load_two_float_operands(stack_var)?;

The _slot1_a is silently ignored. In codegen_inline_float_binary_op, slot1_a is passed to emit_store_float_result — so callers need to understand that in 40-byte mode the store target is slot1 not ptr_a.

A cleaner approach would be to return an opaque FloatStoreTarget or have emit_store_float_result accept ptr_a and compute slot1 internally, keeping the slot offset math inside layout.rs.

4. emit_store_float_result parameter naming

The parameter is called store_target but in 40-byte mode it must be slot1 (not ptr_a). The doc says as much, but the mismatch between what callers pass and what "store target" means at the call site is a footgun. This is related to point 3 above.

---

Minor Nits

• codegen_shift_compute still has // Issue #215: extracted helper in its doc comment (line ~527 of the diff) but the PR removes similar notes elsewhere for consistency.
• The _ptr_b variables computed in emit_load_two_int_operands (used only for their GEP side-effect in some call paths) are correct but worth a comment explaining why ptr_b isn't returned.

---

Summary

The core refactoring direction is correct and the result is clearly better than before. The main things worth addressing before merging:

1. Fix the duplicate doc comment (trivial)
2. Convert todo!() to Err(...) in tagged-ptr float paths (medium — needs a new error variant or reuse of existing)
3. Consider cleaning up the slot1_a leakage from the float operand API (optional, but would make the abstraction cleaner)

Happy to discuss the float API design if helpful.

[claude]

PR Review: Virtual Stack Layout Helpers

This is a clean, well-executed refactoring. The core idea - centralising LLVM IR generation into layout-aware helpers in layout.rs - is the right design. Net -110 lines with better abstraction and dual-mode (40-byte / tagged-ptr) support is a solid outcome.

Strengths

• DRY improvement: repeated 4-5 instruction sequences (GEP to slot1 GEP to load) are now single-call helpers. Readability in every call site is dramatically better.
• Tagged-ptr readiness: branching on self.tagged_ptr inside layout helpers is the right layer for this concern, keeping call sites mode-agnostic.
• Tests: 7 new unit tests in layout.rs cover both modes for int ops and correctly verify the error path for unimplemented float tagged-ptr support.
• VirtualValue::discriminant() removal: no longer needed now that each store helper embeds the discriminant. Correct call.

Issue 1: Duplicate GEP in control_flow.rs

let top_ptr = self.emit_stack_gep(stack_var, -1)?;
let cond_val = self.emit_load_int_payload(&top_ptr)?;
let popped_stack = self.emit_stack_gep(stack_var, -1)?; // identical to top_ptr

top_ptr and popped_stack are identical GEPs (same base, offset -1). This emits two getelementptr instructions to the same address. LLVM deduplicates at -O1+ but it is noise at -O0. Since top_ptr IS the new SP after the pop, reusing it directly drops the redundant call. The original code had this same issue.

Issue 2: Unnecessary else in emit_store_float_bits and emit_store_float_result

Both functions early-return Err in the tagged-ptr branch but still wrap the non-tagged path in an else block. The else is dead after the return. Idiomatic Rust drops it and lets the non-tagged path fall through to Ok(()).

Issue 3: dispatch.rs is the acknowledged remaining gap

60+ occurrences of the same GEP pattern in dispatch.rs are not yet migrated. Fine as a staged approach but worth a follow-up issue to ensure full adoption.

Issue 4: Weakened test assertion in mod.rs is correct but could use a comment

Changed from ir.contains("trunc i64 %result to i32") to the more permissive lines().any() form. Necessary because the SSA name is now a fresh temp rather than the hardcoded %result. A brief comment would help future readers.

Summary

Solid foundation. Issues 1 and 2 are easy fixes. Issues 3 and 4 are awareness items. Great direction overall.

[claude]

PR Review: Virtual Stack Layout Helpers

This is a clean, well-executed refactoring. Layout helpers in layout.rs nicely abstract the 40-byte vs. tagged-pointer representation differences, and the net reduction of ~110 lines while making each call site more readable is a solid win.

Positive observations

• Repeated GEP/load/store patterns across ops.rs, control_flow.rs, statements.rs, and virtual_stack.rs are now consolidated into intent-level helpers.
• Dead code dropped cleanly: VirtualValue::discriminant(), codegen_float_load_operands, codegen_shift_load_operands.
• Err(CodeGenError::Logic) for unimplemented tagged-ptr float paths is the right library-level choice over panicking.
• 7 new tests cover both layout modes and validate error paths.
• Commit message tables confirm prior review feedback was fully addressed.

Issues and suggestions

1. Misleading doc on emit_store_float_bits (layout.rs): The doc says the raw bits are stored and runtime boxing is deferred, but the tagged-ptr branch immediately returns Err and stores nothing. The doc should say: not yet implemented, returns Err; floats will need a runtime boxing call.

2. Missing tests for float helpers in default (40-byte) mode: emit_load_two_float_operands and emit_store_float_result only test the tagged-ptr error path. The 40-byte happy path -- used in production for all float arithmetic/comparisons -- has no unit coverage. A test verifying bitcast + slot1 GEP + store in the emitted IR would catch regressions.

3. ptr_b intent in emit_load_two_int_operands: ptr_b is computed, used to load val_b, then dropped from the return. A short note that ptr_b is not returned because results always write back to ptr_a would help readers understand why the signature is (ptr_a, val_a, val_b) rather than a 4-tuple.

4. dispatch.rs gap: 60+ occurrences remain for a follow-up PR. A tracking issue with a reference in the PR body or a TODO comment at the top of dispatch.rs would prevent this from being forgotten.

Minor nits

• control_flow.rs: popped_stack = top_ptr.clone() with the SP comment is clear and correct.
• mod.rs test: ir.lines().any(...) is the right trade-off for dynamic SSA names; the explanatory comment helps.

Summary

Ready to merge. The missing 40-byte float tests (item 2) are the most impactful gap; the rest is documentation and tracking hygiene. Ideal to add those tests before merge, but including them in the dispatch.rs follow-up is also reasonable.