-
Notifications
You must be signed in to change notification settings - Fork 732
performance(casm): Made casm builder build-alg linear. #9489
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
performance(casm): Made casm builder build-alg linear. #9489
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
eytan-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests checking multiple branches, and something that tests future_label
@eytan-starkware reviewed all commit messages and made 9 comments.
Reviewable status: 0 of 1 files reviewed, 8 unresolved discussions (waiting on @orizi and @TomerStarkware).
crates/cairo-lang-casm/src/builder.rs line 104 at r1 (raw file):
/// The index of the instruction that needs to be relocated. instruction_index: usize, /// The code offset if the instruction to be relocated.
Fix comment
crates/cairo-lang-casm/src/builder.rs line 121 at r1 (raw file):
state: State, /// The offset of the label. If not set means not yet reached (and not read-only). offset: Offset,
If it might not yet be set shouldnt it just be option?
crates/cairo-lang-casm/src/builder.rs line 128 at r1 (raw file):
impl Offset { /// The value for an unset offset. const UNSET: Offset = Offset(usize::MAX);
Should be u32, as later you assume it fits i32. Even better to keep it i32
crates/cairo-lang-casm/src/builder.rs line 172 at r1 (raw file):
let mut branch_relocations: [Vec<usize>; BRANCH_COUNT] = core::array::from_fn(|_| Vec::new()); for r in self.relocations {
relocation
Code quote:
rcrates/cairo-lang-casm/src/builder.rs line 179 at r1 (raw file):
relocate_instruction( &mut self.instructions[r.instruction_index], label_offset.0 as i32 - r.instruction_offset as i32,
I assume this will never overflow, but how big can we get here?
crates/cairo-lang-casm/src/builder.rs line 442 at r1 (raw file):
); let state = core::mem::take(&mut self.main_state); self.set_or_test_label(label, state);
You are taking state and not returning it on purpose?
crates/cairo-lang-casm/src/builder.rs line 742 at r1 (raw file):
/// Relocates an instruction by updating its jump offset. fn relocate_instruction(instruction: &mut Instruction, updated: i32) {
#[inline(always)]
fn relocate_instruction(
Code quote:
fn relocate_instruction(crates/cairo-lang-casm/src/builder.rs line 749 at r1 (raw file):
}) | InstructionBody::Jump(JumpInstruction { target: DerefOrImmediate::Immediate(value),
Does jump target really need to be bigint?
b894b20 to
aa71e7f
Compare
orizi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orizi made 8 comments.
Reviewable status: 0 of 1 files reviewed, 8 unresolved discussions (waiting on @eytan-starkware and @TomerStarkware).
crates/cairo-lang-casm/src/builder.rs line 104 at r1 (raw file):
Previously, eytan-starkware wrote…
Fix comment
Done.
crates/cairo-lang-casm/src/builder.rs line 121 at r1 (raw file):
Previously, eytan-starkware wrote…
If it might not yet be set shouldnt it just be option?
the size of the object here seems to have a non-negligible effect.
crates/cairo-lang-casm/src/builder.rs line 128 at r1 (raw file):
Previously, eytan-starkware wrote…
Should be u32, as later you assume it fits i32. Even better to keep it i32
it is created from usize - as it is the sum of usizes - checked on conversion - instead of on every add.
crates/cairo-lang-casm/src/builder.rs line 172 at r1 (raw file):
Previously, eytan-starkware wrote…
relocation
Done.
crates/cairo-lang-casm/src/builder.rs line 179 at r1 (raw file):
Previously, eytan-starkware wrote…
I assume this will never overflow, but how big can we get here?
very very small - this is just for creating a single libfunc code.
crates/cairo-lang-casm/src/builder.rs line 442 at r1 (raw file):
Previously, eytan-starkware wrote…
You are taking state and not returning it on purpose?
it is a jump code - the current state is the default (no vars) and unreachable.
crates/cairo-lang-casm/src/builder.rs line 742 at r1 (raw file):
Previously, eytan-starkware wrote…
#[inline(always)]
fn relocate_instruction(
rather let the compiler do its thing - no reason for me to do that.
crates/cairo-lang-casm/src/builder.rs line 749 at r1 (raw file):
Previously, eytan-starkware wrote…
Does jump target really need to be bigint?
not up to us - especially not in this PR.
SIERRA_UPDATE_PATCH_CHANGE_TAG=No interface changes.
aa71e7f to
99a862b
Compare
orizi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are already multiple tests with multiple branches.
added future_label test.
@orizi made 1 comment.
Reviewable status: 0 of 1 files reviewed, 8 unresolved discussions (waiting on @eytan-starkware and @TomerStarkware).
eytan-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eytan-starkware reviewed 2 files and all commit messages, made 2 comments, and resolved 7 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi and @TomerStarkware).
crates/cairo-lang-casm/src/builder.rs line 742 at r1 (raw file):
Previously, orizi wrote…
rather let the compiler do its thing - no reason for me to do that.
I would say if we are sure we could inline it (as this just simplifies code), we should help the compiler out
orizi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orizi resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @TomerStarkware).
TomerStarkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TomerStarkware made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved.

Summary
Refactored the CASM builder to improve label handling and instruction relocation. The PR replaces the
Statementenum with a more direct approach that separates instructions from relocations, making the code more maintainable. It introduces a dedicatedLabelRelocationstruct andOffsettype to track and apply relocations more efficiently, eliminating the need to compute label offsets in a separate pass.Type of change
Please check one:
Why is this change needed?
The previous implementation used a complex approach with a
Statementenum that mixed instructions, jumps, and labels. This required a separate pass to compute label offsets and apply relocations. The new implementation is more direct, tracking instructions and relocations separately, which simplifies the code and improves performance by eliminating the need for the extra computation pass.What was the behavior or documentation before?
The builder used a
Statementenum with three variants (Final, Jump, Label) and required a separate pass to compute label offsets before applying relocations to jump instructions.What is the behavior or documentation after?
The builder now directly stores instructions and tracks relocations separately with a dedicated
LabelRelocationstruct. Label offsets are tracked in theLabelInfostruct with anOffsettype, allowing for more efficient relocation handling without requiring a separate computation pass.Additional context
This refactoring maintains the same functionality while making the code more maintainable and potentially more efficient by eliminating unnecessary computation steps. The change is internal to the CASM builder and doesn't affect its public API.