Refactor stackify - #299
Merged
Merged
Conversation
Delete the never-called traverse_edge; rename read/write/read_br_table_case to pre_inst/post_inst/br_table_case and drop the ignored operand params; take &dyn Allocator instead of &mut dyn; rewrite FinalAlloc action lists once at construction so lookups return borrows; factor the spill-storage store-action mapping into SpillStorage::store_action.
Delete the spill_obj and scratch_slot_of_value projection fields; derive them via accessors (spill_obj, scratch_slot, object_spills) with a single set_spill_object mutation path. The provisional per-iteration object-id map moves out of StackifyAlloc into the planning driver. The map-drift assertions in validate_spill_storage become unrepresentable.
- Replace three confirmed-unreachable branches with debug asserts: the dead-value defensive loop in clean_dead_stack_prefix, the dominated-pred arm in choose_transfer, and the plan_block prologue fallback; clear_inst_actions becomes an emptiness assertion. - Express StackifySearchProfile knobs as SearchBudgets const tables. - terminal_chain_blocks becomes a BitSet. - Drop the inst collect in run_block_sim (iterate the layout directly). - Share the storage-eligibility predicate between MemPlan and the builder's entry-arg pre-pass. - Document the SWAP-chain cost of the stable SymStack ops; drop the imm_materialization_code_len alias.
The machine pipeline ran only CriticalEdgeSplitter, so a multiway self-loop on the entry block tripped a debug assert (or silently dropped the edge fixup in release). Run StackifyEdgeSplitter in prepare_machine_stackify_analysis instead, and: - Fix a latent CfgEditor::split_edge bug: inserting the split block before an entry-block destination silently made it the new entry, recreating the multiway-edge-to-planned-block problem. - Narrow the splitter from all in-cycle multiway edges to retreating ones (dom-RPO rank[to] <= rank[from]) — exactly the planner's assert condition — so existing corpus bytecode is byte-identical. - Move stackify_edge.rs to stackalloc/edge_split.rs; document the precondition; upgrade the planner guards to release asserts. - Add an end-to-end regression test for the entry self-loop shape.
StackifyTrace records structured TraceEvents instead of interleaving placeholder markers into one string and substituting at render time. Rendering is a single forward walk; checkpoint/rollback is a vec truncate; deferred-exit actions fill in by event index; object-id remapping rewrites raw Action payloads in events. The observer trait loses both associated types. Rendered output is byte-identical.
Emit and the lazy-frame analysis read a br_table's pre-actions via pre_inst like every other instruction; brtable_actions now hold only per-case compare preparation. This removes the take/prefix threading through PlannerActionSink. The lazy-frame planner bails to the always-active frame when br_table base pre-actions touch the frame, since emit replays their action indexes once per case.
Planner::prepare_internal_call now owns the operand-rotation + continuation-push + single-SWAP sequence with the ABI documented in one place; the rotation site and SymStack helpers cross-reference it. remove_call_ret_addr's linear marker scan becomes pop_call_ret_addr, asserting the continuation is on top (it is, by construction).
- UseTracker (uses.rs) owns per-block use counting, cached-immediate tracking, and last-use/preserve queries; BlockLiveSets is gone. - rescue.rs owns dead-prefix cleanup and reachability-rescue heuristics; the ReachabilityValues wrapper is gone. - MemState groups the driver's memory-planning borrows; MemPlan::new drops from nine args to four and with_planner constructs it once. - BlockPlanner (block.rs, was block_sim.rs) owns the per-block walk with one method per terminator kind; observer calls live at one layer; edge bookkeeping stays on the driver as record_*_edge. All four take_stack dances are gone. - IterationPlanner is renamed FunctionPlanner and lives in driver.rs.
New entry.rs owns the per-block EntryKind classification (function entry / terminal-chain opaque / single-pred inherited / merge) and the Pending -> Frozen entry lifecycle. EntryTable::record_edge is the one edge-dispatch point and EntryTable::freeze_entry the one transfer- selection point; frozen templates live in the entry state. This replaces the templates / inherited_stack / pending_edges / planned_blocks / terminal_chain side tables, and double-freezing a template is now structurally unreachable instead of silently ignored. plan_block becomes a short dispatch over ResolvedEntry.
Stackify already treats equal as_i256 words as interchangeable stack items (rename_immediate_slots_to_match, semantic suffix matching). Give each distinct word one representative ValueId via the existing value-alias mechanism, choosing the class member with the cheapest materialization (then lowest id) so a class never pushes worse bytes than its best member. This is stage 1 of folding the parallel immediate-identity machinery into ordinary ValueId equality.
With one canonical ValueId per immediate word, plain equality subsumes the word-level matching: delete rename_immediate_slots_to_match, common_suffix_len_semantic, operand_prep_item_matches_arg, and unary_operand_prep_find_arg; plan-replay mismatches still roll back to the greedy fallback, and flush_rebuild asserts exact equality.
With one canonical ValueId per immediate word, UseTracker's parallel I256-keyed cached_imm_remaining map merges into the ValueId-keyed remaining map: cached immediates enter live_future until their last in-block use, cleanup_live reduces to live_future, and cache_preserve becomes the plain not-a-last-use rule. Rescue keeps immediates evictable regardless of liveness, preserving its prior behavior.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e9fcdcd7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
@codex review |
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.
Refactor the EVM stackify pipeline to make its core state and interfaces explicit, remove duplicated bookkeeping, and close several latent correctness gaps.
ValueIdidentity per immediate stack word;Simplify allocator and storage boundaries
read/write/read_br_table_casewith borrowedpre_inst/post_inst/br_table_caseaction lists.spill_storagethe single stored representation of scratch, object, and exact-local spill locations.MemPlanconstruction boundary.Reorganize stackify planning
BlockPlanner.EntryTable.Pending -> Frozenlifecycle.prepare_internal_call.Harden control-flow handling
StackifyEdgeSplitterfrom the machine preparation pipeline.br_tablebase pre-actions separate from per-case comparison actions.br_tablelazy-frame action handling.Simplify immediate identity
ValueId.Simplify tracing