fix: P1 ownership-pass core for m8.4.2 (I-045+I-069, I-066, I-064+I-065, I-107+I-110, I-119, I-090, I-108) - #107
Merged
Conversation
analyze_loop_body now runs in two phases: - Phase 1 propagates state only: each walk uses a scratch DiagSink AND a staging sidecar clone, so speculative diagnostics and sidecar writes are discarded per iteration (I-069 — diverged loops no longer leak branch-gated FreePoints and if_branches entries gated on dead BranchIds). Bounded to two walks: merge_non_monotone's binding-aware override is non-monotone under consume-then-rebind (flips Moved back to Valid each merge), so an unbounded loop oscillates forever; the cap reproduces the historical 2-pass precision. Documented at the cap and on analyze_while_loop. - Phase 2 runs a single check pass against the real sink/sidecar from the propagated lattice, so diagnostics are always derived from the same state instead of whichever speculative iteration emitted them (I-045). TDD: diverged_loop_writes_branch_gated_free_exactly_once failed red (2 branch-gated FreePoints, one on a dead BranchId), now green. Verified: ryo-frontend 395, ryo-core 62, ryo-backend 9, integration 187/187, clippy clean; byte-identical output vs the pre-refactor compiler on all 16 examples plus the consume-then-rebind repro. Removes I-045 and I-069 from ISSUES.md.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
walk_operands + ChildKind, loop_body, collect_loop_body_refs, collect_reachable (was collect_refs_recursive), and collect_jump_path now live in ryo-core/src/tir.rs as Tir methods — a single structural- reachability surface next to the TIR views, instead of private helpers in the ownership pass re-encoding the same shape dispatch. ownership.rs call sites updated; pure move, no behavior change. Verified: ryo-core 62, ryo-frontend 395, ryo-backend 9, integration 187/187, clippy clean. Removes I-066 from ISSUES.md.
I-064: inside_loop and has_any were rebuilt on every break/continue jump — K full body traversals + K free_schedule scans for K jumps. Both are invariant across jumps in the same loop; they now live in a LoopExitCtx built once in schedule_loop_exit_frees_in's loop arm and passed down by reference. covers_this_jump/on_path stay per-jump. (Jump-exit scheduling only appends Frees anchored inside the loop, which the per-jump free_inside_loop check already accounts for, so the has_any snapshot at loop entry stays exact.) I-065: collect_jump_path walked every if-arm and body-prefix stmt into throwaway HashSets just for containment — O(N^2) with N allocations. (a) New Tir::contains_reachable: allocation-free short-circuiting DFS, now used for arm selection. (b) LoopExitCtx precomputes a ref -> top-level body stmt map; the jump's enclosing stmt is one lookup and only prefix stmts are walked, with collect_jump_path descending from the single enclosing stmt. Verified: ryo-core 62, ryo-frontend 395, ryo-backend 9, integration 187/187, clippy clean. Removes I-064 and I-065 from ISSUES.md.
I-107: four hot-path sites did tir.params.iter().position(...).expect() — O(P) per call inside per-owner loops. Ownership now carries a name->param-index map built once per function in analyze_function; Owner::tirref and the three standalone lookups consult it. Panic semantics kept as an internal invariant. I-110: the tree-shaped-TIR assumption behind find_consumers' first-parent-wins lived in one comment. It is now documented on Tir itself and checked by Tir::validate_tree_shape (debug-only, called from TirBuilder::finish) — each inst must have at most one parent. The assertion caught one hand-built DAG in a tir.rs unit test; fixed to match sema's fresh-Var-per-read convention. Verified: ryo-core 62, ryo-frontend 395, ryo-backend 9, integration 187/187, clippy clean. Removes I-107 and I-110 from ISSUES.md.
loop_nesting_of walked the whole function body per query (per view/read pair, per arm-refinement candidate, per materialize site). Ownership now carries a walk-constant loop_nesting map — every instruction -> its enclosing WhileLoop/ForRange stack, outermost first — built once per function and consulted via nesting_of. Cond/bounds-counts-as-inside and nested-loop-deeper-stack semantics preserved; a missing key degrades to the old function's exact answer (empty stack). Verified: ryo-core 62, ryo-frontend 395, ryo-backend 9, integration 187/187, clippy clean. Removes I-119 from ISSUES.md.
merge_branches (N-way, if-arms) and merge_non_monotone (2-way, loop fixed-point) re-implemented the same any-Moved-wins state merge, binding-aware override, current_owner first-wins, dead-store intersect/union, and live-projections union. Extracted five helpers — merge_states_any_moved_wins, merge_current_owner_first_wins, merge_binding_states, merge_pending_dead_store, union_live_projections — and expressed both merges through them. Semantic differences catalogued and preserved: field coverage (monotone fields only in the N-way), override side sets, and the deliberately non-monotone override the loop propagate cap depends on. Stale merge_two comment references updated. Verified: ryo-core 62, ryo-frontend 395, ryo-backend 9, integration 187/187, clippy clean. Removes I-090 from ISSUES.md.
A still-Valid Owner::Param at function exit was freed after the last body statement — coarser than the local-variable last-use policy. Param-owned reads now register in last_use under the param's sentinel ref (Owner::tirref via the param-index map; Inst keying unchanged), and the Param arm of the last-use pass reuses the Inst anchoring logic: defer_anchor for P5 projection deferral, and outermost_branch_of + branch_may_not_return for last uses inside a branch/loop (an in-loop read anchor would fire per iteration — UAF; anchoring after the outermost branch is leak-safe on not-taken paths). Never-read params keep the last-stmt anchor; the inout escape exclusion is untouched. No existing test expectations changed; four new tests pin the cases. Verified: ryo-core 62, ryo-frontend 399, ryo-backend 9, asan_smoke 27/27, integration 187/187, clippy clean. Removes I-108 from ISSUES.md.
artefactop
changed the title
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
…/p1-ownership-core
Contributor
Merging this PR will degrade performance by 10.38%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | lex_large[64] |
406.9 µs | 454 µs | -10.38% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/p1-ownership-core (06066f2) with fix/p0-sret-terminator (626b5da)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #106 — retarget to
mainonce that merges.Summary
P1 tier of the m8.4.2 issue audit, one commit per issue, all in the ownership pass / TIR reachability:
analyze_loop_bodyrestructured into propagate-then-check: Phase 1 propagates state only (scratch sink + staging sidecar, discarded per iteration), Phase 2 runs a single check pass against the real sink/sidecar from the propagated lattice. Diverged loops no longer leak branch-gatedFreePoints/if_branchesentries gated on deadBranchIds, and diagnostics always come from the converged lattice. Phase 1 is capped at 2 walks becausemerge_non_monotone's binding-aware override is non-monotone under consume-then-rebind (unbounded iteration oscillated — found and root-caused during this work; documented at the cap and onanalyze_while_loop).walk_operands/ChildKind,loop_body,collect_loop_body_refs,collect_reachable,collect_jump_pathmoved toryo-core/src/tir.rsasTirmethods: one structural-reachability surface instead of private helpers re-encoding shape dispatch.LoopExitCtxprecomputesinside_loop/has_any/ref→top-level-stmt map once per loop instead of per jump; new allocation-freeTir::contains_reachablereplaces throwaway-HashSet arm probes incollect_jump_path.Ownership.param_indexmap replaces four linear panicking param lookups; tree-shaped-TIR invariant documented onTirand debug-checked byTir::validate_tree_shape(caught one hand-built DAG in a unit test).Ownership.loop_nestingmemoizes nesting stacks in a single pre-pass instead of per-query body walks.Verification (per commit)
ryo-frontend399/399,ryo-core62/62,ryo-backend9/9, integration 187/187, asan_smoke 27/27, clippy cleanexamples/*.ryo(checked at the I-045+I-069 commit)Removes I-045, I-064, I-065, I-066, I-069, I-090, I-107, I-108, I-110, I-119 from ISSUES.md.