-
Notifications
You must be signed in to change notification settings - Fork 188
fill holes is builtins segments #2036
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
fill holes is builtins segments #2036
Conversation
Benchmark Results for unmodified programs 🚀
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## starkware-development #2036 +/- ##
=========================================================
- Coverage 96.54% 96.51% -0.03%
=========================================================
Files 102 102
Lines 42755 42834 +79
=========================================================
+ Hits 41276 41340 +64
- Misses 1479 1494 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 2 of 3 files reviewed, 7 unresolved discussions (waiting on @Stavbe)
vm/src/vm/vm_core.rs
line 755 at r1 (raw file):
pub fn complete_builtin_auto_deductions(&mut self) -> Result<(), VirtualMachineError> { for builtin in self.builtin_runners.iter() { let index: usize = builtin.base();
move down and rename
Suggestion:
let builtin_index: usize = builtin.base();
vm/src/vm/vm_core.rs
line 770 at r1 (raw file):
MemoryCell::NONE, ); }
Skip the irrelevant builtins and put real logic below
Suggestion:
if matches!(
builtin,
BuiltinRunner::Output(_) | BuiltinRunner::SegmentArena(_)
) {
continue;
}
vm/src/vm/vm_core.rs
line 773 at r1 (raw file):
// Collect the values that need to be written to the memory. let mut new_values: Vec<(usize, MemoryCell)> = vec![];
Nit
Suggestion:
let mut missing_values: Vec<(usize, MemoryCell)> = vec![];
vm/src/vm/vm_core.rs
line 784 at r1 (raw file):
{ let memory_value = cell.get_value(); if memory_value.is_some() {
Add comment
Suggestion:
// Checks that the value in the memory is correct.
if memory_value.is_some() {
vm/src/vm/vm_core.rs
line 784 at r1 (raw file):
{ let memory_value = cell.get_value(); if memory_value.is_some() {
Consider to change to a match statement
Code quote:
if memory_value.is_some() {
vm/src/vm/vm_core.rs
line 791 at r1 (raw file):
} } else { new_values.push((offset, MemoryCell::new(deduced_memory_cell)));
same
Suggestion:
// Collect value to be stored in memory.
new_values.push((offset, MemoryCell::new(deduced_memory_cell)));
vm/src/vm/vm_core.rs
line 796 at r1 (raw file):
} for (offset, value) in new_values {
rename and add comment above
Suggestion:
for (address, value) in new_values {
59f10c9
to
c373626
Compare
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.
Reviewable status: 1 of 3 files reviewed, 5 unresolved discussions (waiting on @shaharsamocha7)
vm/src/vm/vm_core.rs
line 755 at r1 (raw file):
Previously, shaharsamocha7 wrote…
move down and rename
Done.
vm/src/vm/vm_core.rs
line 784 at r1 (raw file):
Previously, shaharsamocha7 wrote…
Add comment
Done.
vm/src/vm/vm_core.rs
line 791 at r1 (raw file):
Previously, shaharsamocha7 wrote…
same
Done.
vm/src/vm/vm_core.rs
line 796 at r1 (raw file):
Previously, shaharsamocha7 wrote…
rename and add comment above
I think offset is more correct because it is not relocated to addresses at this point; only segment index and offset
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.
Reviewed 1 of 1 files at r2.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @Stavbe)
vm/src/vm/vm_core.rs
line 796 at r1 (raw file):
Previously, Stavbe wrote…
I think offset is more correct because it is not relocated to addresses at this point; only segment index and offset
ok
4532fff
to
7780af2
Compare
|
190d2c2
to
f55117c
Compare
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.
Reviewable status: 2 of 5 files reviewed, all discussions resolved (waiting on @Stavbe)
f55117c
to
2721498
Compare
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.
Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @Stavbe)
vm/src/tests/memory_comparator.py
line 26 at r5 (raw file):
print(f'{k}:{cairo_mem[k]}') mismatched = True
remove spaces
34b9a28
to
20f0dfa
Compare
f0f4db7
to
9fc7999
Compare
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.
Reviewable status: 1 of 5 files reviewed, 1 unresolved discussion (waiting on @DavidLevitGurevich and @shaharsamocha7)
vm/src/tests/memory_comparator.py
line 26 at r5 (raw file):
Previously, DavidLevitGurevich wrote…
remove spaces
Done.
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.
Reviewed 1 of 2 files at r4, 1 of 3 files at r7, all commit messages.
Reviewable status: 3 of 5 files reviewed, 2 unresolved discussions (waiting on @JulianGCalderon, @shaharsamocha7, and @Stavbe)
vm/src/tests/memory_comparator.py
line 12 at r7 (raw file):
compare_memory_file_contents(f1.read(), f2.read()) def compare_memory_file_contents(cairo_raw_mem, cairo_rs_raw_mem):
what does this function do now?
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.
Reviewable status: 3 of 5 files reviewed, 2 unresolved discussions (waiting on @DavidLevitGurevich, @JulianGCalderon, and @shaharsamocha7)
vm/src/tests/memory_comparator.py
line 12 at r7 (raw file):
Previously, DavidLevitGurevich wrote…
what does this function do now?
Nothing, But I didn't want to remove all the calls for it because maybe we will add it back after the new flow will be fully supported, wdyt?
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.
Reviewed 2 of 3 files at r7.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @JulianGCalderon and @Stavbe)
vm/src/tests/memory_comparator.py
line 12 at r7 (raw file):
Previously, Stavbe wrote…
Nothing, But I didn't want to remove all the calls for it because maybe we will add it back after the new flow will be fully supported, wdyt?
add a Huge TODO, and don't totally delete the code, only disable it
vm/src/vm/runners/cairo_runner.rs
line 5662 at r7 (raw file):
.get_prover_input_info() .expect("Failed to get prover input info"); assert!(prover_input.builtins_segments.get(&6) == Some(&BuiltinName::bitwise));
add another check that also something that is not the last was filled
9fc7999
to
9200100
Compare
9200100
to
2137db8
Compare
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.
Reviewable status: 3 of 5 files reviewed, 3 unresolved discussions (waiting on @DavidLevitGurevich and @JulianGCalderon)
vm/src/tests/memory_comparator.py
line 12 at r7 (raw file):
Previously, DavidLevitGurevich wrote…
add a Huge TODO, and don't totally delete the code, only disable it
Done.
vm/src/vm/runners/cairo_runner.rs
line 5662 at r7 (raw file):
Previously, DavidLevitGurevich wrote…
add another check that also something that is not the last was filled
Added a todo at the beginning of the test
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.
Reviewed 1 of 3 files at r1, 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JulianGCalderon)
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JulianGCalderon)
Hi @Stavbe! Could you add a description telling the purpose of this PR? |
TITLE
Description
Description of the pull request changes and motivation.
Checklist
This change is