Conversation
|
This change is |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
PR SummaryMedium Risk Overview Forward dataflow ( Inlining ( Reviewed by Cursor Bugbot for commit 37d12f5. Bugbot is set up for automated code reviews on this repo. Configure here. |
125cfd6 to
a8944a3
Compare
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 5 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on orizi).
crates/cairo-lang-lowering/src/analysis/forward.rs line 57 at r1 (raw file):
// Get entry info from incoming edges. Can move out, since we are working in // lexicographic order.
topological
crates/cairo-lang-lowering/src/inline/mod.rs line 232 at r1 (raw file):
// Seed the builder by moving the existing blocks in (they are discarded at the end of this // function when `lowered.blocks` is overwritten), rather than deep-cloning each block. let mut blocks: BlocksBuilder<'db> = lowered.blocks.into_builder();
Did you consider something like
let Lowered { blocks, variables, .. } = lowered;
instead of blocks builder? Removes the mem::take and the unsafety that comes with it
crates/cairo-lang-lowering/src/objects/blocks.rs line 130 at r1 (raw file):
/// place. Used by passes that rebuild the block list from scratch, to avoid cloning every /// block when the original is discarded anyway. pub fn into_builder(&mut self) -> BlocksBuilder<'db> {
Into is a bad name for taking &mut
a8944a3 to
ea673ed
Compare
orizi
left a comment
There was a problem hiding this comment.
@orizi made 3 comments.
Reviewable status: 2 of 5 files reviewed, 3 unresolved discussions (waiting on eytan-starkware).
crates/cairo-lang-lowering/src/analysis/forward.rs line 57 at r1 (raw file):
Previously, eytan-starkware wrote…
topological
Done.
crates/cairo-lang-lowering/src/inline/mod.rs line 232 at r1 (raw file):
Previously, eytan-starkware wrote…
Did you consider something like
let Lowered { blocks, variables, .. } = lowered;
instead of blocks builder? Removes the mem::take and the unsafety that comes with it
Done.
crates/cairo-lang-lowering/src/objects/blocks.rs line 130 at r1 (raw file):
Previously, eytan-starkware wrote…
Into is a bad name for taking &mut
Done.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ea673ed. Configure here.
…ning and dataflow.
Reduce heap allocations in the lowering pipeline by removing redundant copies:
- inline: seed the BlocksBuilder by moving the existing blocks in via the new
`Blocks::into_builder` (mem::take) instead of deep-cloning every block, since
`lowered.blocks` is overwritten at the end of the pass anyway.
- analysis/backward: back the per-block info cache with a `Vec<Option<Info>>`
indexed by `BlockId` instead of an `UnorderedHashMap`, dropping the hashing
and per-entry allocation.
- analysis/forward: move the incoming info out with `take()` rather than
`clone()`, since each block's slot is dead once the block is processed.
Measured on corelib -> Sierra (dhat): -33.7 MB allocated (-3.7%) and -140,744
allocations (-2.2%); peak heap unchanged.
ea673ed to
37d12f5
Compare
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 3 files and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on orizi).
Summary
Replaces the
UnorderedHashMap<BlockId, TAnalyzer::Info>inBackAnalysiswith aVec<Option<TAnalyzer::Info>>indexed directly byBlockId.0, usingis_none()/take()/index-assignment instead ofcontains_key/remove/insert. Adds aBlocks::into_buildermethod that moves blocks out of aBlocks<'db>into aBlocksBuilderviastd::mem::take, and uses it ininner_apply_inliningto avoid deep-cloning every block before discarding the original. Also moves from.clone().unwrap()to.take().unwrap()in the forward analysis where lexicographic ordering guarantees each entry is consumed exactly once.Type of change
Please check one:
Why is this change needed?
BackAnalysiswas using a hash map for block info storage despiteBlockIdbeing a dense integer index, making lookups and membership checks more expensive than necessary. Ininner_apply_inlining, every block was being cloned into the newBlocksBuildereven though the originallowered.blockswas immediately overwritten and the clones discarded, wasting allocations proportional to the size of the function being inlined.What was the behavior or documentation before?
BackAnalysisstored per-block analysis results in anUnorderedHashMap<BlockId, TAnalyzer::Info>, using hash-based lookups for cache checks and insertions.inner_apply_inliningcalled.clone()on every block to seed theBlocksBuilder, then discarded the originals.What is the behavior or documentation after?
BackAnalysisstores per-block analysis results in aVec<Option<TAnalyzer::Info>>indexed byBlockId.0, with O(1) direct-index access and no hashing overhead.inner_apply_inliningmoves blocks out oflowered.blocksviaBlocks::into_builder, eliminating the per-block clone.Related issue or discussion (if any)
Additional context
The
Vec-based approach is valid becauseBlockIdvalues are dense indices intolowered.blocks, so theVecis pre-sized to exactlylowered.blocks.len()with no wasted capacity.