-
Notifications
You must be signed in to change notification settings - Fork 166
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
Ohadn/qm31 operations #1938
base: ohadn/qm31_arithmetics-in-math_utils
Are you sure you want to change the base?
Ohadn/qm31 operations #1938
Conversation
|
e15c6e6
to
1d1be8b
Compare
Benchmark Results for unmodified programs 🚀
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## ohadn/qm31_arithmetics-in-math_utils #1938 +/- ##
========================================================================
+ Coverage 96.41% 96.45% +0.03%
========================================================================
Files 102 103 +1
Lines 41858 42282 +424
========================================================================
+ Hits 40358 40781 +423
- Misses 1500 1501 +1 ☔ View full report in Codecov by Sentry. |
efdb5e3
to
6dd3870
Compare
b4da427
to
eb339d8
Compare
eb339d8
to
772ae29
Compare
d6d9a96
to
d1f230b
Compare
772ae29
to
074772b
Compare
d1f230b
to
4bca3db
Compare
074772b
to
06b5260
Compare
4bca3db
to
ea9f93f
Compare
c9356e0
to
c13233c
Compare
ea9f93f
to
ef1aa46
Compare
c13233c
to
bd6140f
Compare
6e168b8
to
ada9240
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.
Reviewed 1 of 6 files at r4.
Reviewable status: 9 of 16 files reviewed, 2 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @ohad-nir-starkware, @Oppen, @pefontana, and @yuvalsw)
3ef056d
to
286ffff
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: 8 of 17 files reviewed, 2 unresolved discussions (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, and @yuvalsw)
vm/src/vm/vm_core.rs
line 504 at r3 (raw file):
Previously, DavidLevitGurevich wrote…
why did this make sense but with the QM31 we need a totally different approach? @JulianGCalderon
I refactored deduce_op0, deduce_op1, compute_res
, the flow is now more uniform for Stone
and QM31Operation
.
However, it did import OpcodeExtension
to relocatable.rs
which is something I feel unsure about.
Also, in deduce_op0
and deduce_op1
for Res::Mul
the flows of Stone
and QM31Operation
do diverge within the function itself because it already deconstructs Felts
out of MaybeRelocatable
making it unsuitable to be computed inside relocatable.rs
.
I could place the computation inside math_utils
but it will introduce OpcodeExtension
to math_utils
and also separate the computation of div
from that of add, sub, mul
.
Another option is to open another utils file specifically for those 4 functions or to place them inside vm_core.rs
.
let me know what you think.
vm/src/vm/decoding/decoder.rs
line 121 at r3 (raw file):
Previously, DavidLevitGurevich wrote…
ap_update
Done.
286ffff
to
e7227fb
Compare
9cdce6f
to
57521d5
Compare
9238c0c
to
afc8d8f
Compare
57521d5
to
29ce31e
Compare
afc8d8f
to
8074661
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.
Reviewed 1 of 6 files at r4, 1 of 6 files at r5.
Reviewable status: 10 of 17 files reviewed, 1 unresolved discussion (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, @pefontana, and @yuvalsw)
c768e1e
to
ea5d726
Compare
vm/src/vm/decoding/decoder.rs
Outdated
if (res != Res::Add && res != Res::Mul) | ||
|| op1_addr == Op1Addr::Op0 | ||
|| pc_update != PcUpdate::Regular | ||
|| opcode != Opcode::AssertEq | ||
|| (ap_update_num != 0 && ap_update_num != 2) | ||
{ | ||
return Err(VirtualMachineError::InvalidQM31AddMulFlags(flags & 0x7FFF)); | ||
} | ||
OpcodeExtension::QM31Operation |
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.
Could you move this check logic down, to be more consistent with what is done with the Blake checks?
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.
Done.
#[test] | ||
#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] | ||
fn decode_qm31_operation_invalid_flags() { | ||
// opcode_extension| opcode|ap_update|pc_update|res_logic|op1_src|op0_reg|dst_reg | ||
// 79 ... 17 16 15| 14 13 12| 11 10| 9 8 7| 6 5|4 3 2| 1| 0 | ||
// QM31Operation| CALL| REGULAR| JumpRel| Op1| FP| AP| AP | ||
// 1 1 0 0 1 0 0 0 1 0 0 0 0 1 0 0 0 | ||
// 1 1001 0001 0000 1000 = 0x19108; off0 = 1, off1 = 1 | ||
let error = decode_instruction(0x19108800180018001); | ||
assert_matches!( | ||
error, | ||
Err(VirtualMachineError::InvalidQM31AddMulFlags(0x1108)) | ||
); |
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.
Could you add an additional test for the non-error path?
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.
Done.
29ce31e
to
d733ade
Compare
7a9ae5a
to
7925df2
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: 9 of 19 files reviewed, 3 unresolved discussions (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, and @yuvalsw)
vm/src/vm/decoding/decoder.rs
Outdated
if (res != Res::Add && res != Res::Mul) | ||
|| op1_addr == Op1Addr::Op0 | ||
|| pc_update != PcUpdate::Regular | ||
|| opcode != Opcode::AssertEq | ||
|| (ap_update_num != 0 && ap_update_num != 2) | ||
{ | ||
return Err(VirtualMachineError::InvalidQM31AddMulFlags(flags & 0x7FFF)); | ||
} | ||
OpcodeExtension::QM31Operation |
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.
Done.
#[test] | ||
#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] | ||
fn decode_qm31_operation_invalid_flags() { | ||
// opcode_extension| opcode|ap_update|pc_update|res_logic|op1_src|op0_reg|dst_reg | ||
// 79 ... 17 16 15| 14 13 12| 11 10| 9 8 7| 6 5|4 3 2| 1| 0 | ||
// QM31Operation| CALL| REGULAR| JumpRel| Op1| FP| AP| AP | ||
// 1 1 0 0 1 0 0 0 1 0 0 0 0 1 0 0 0 | ||
// 1 1001 0001 0000 1000 = 0x19108; off0 = 1, off1 = 1 | ||
let error = decode_instruction(0x19108800180018001); | ||
assert_matches!( | ||
error, | ||
Err(VirtualMachineError::InvalidQM31AddMulFlags(0x1108)) | ||
); |
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.
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 6 files at r4, 3 of 7 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: 15 of 19 files reviewed, 4 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, and @yuvalsw)
vm/src/vm/vm_core.rs
line 2616 at r7 (raw file):
#[test] #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] fn compute_res_qm31_add_relocatable_values() {
we need more cases covered
vm/src/vm/decoding/decoder.rs
line 128 at r7 (raw file):
} let qm31_operation_flags_invalid = (res != Res::Add && res != Res::Mul)
please change to ..._valid
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 3 of 7 files at r6.
Reviewable status: 18 of 19 files reviewed, 5 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @ohad-nir-starkware, @Oppen, @pefontana, and @yuvalsw)
vm/src/typed_operations.rs
line 22 at r7 (raw file):
) -> Result<MaybeRelocatable, VirtualMachineError> { match opcode_extension { OpcodeExtension::Stone => Ok(x.add(y)?),
why not just
OpcodeExtension::Stone => x.add(y),
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 11 files at r3.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @ohad-nir-starkware, @Oppen, @pefontana, and @yuvalsw)
cairo_programs/stwo_exclusive_programs/qm31_opcodes_test.cairo
line 80 at r7 (raw file):
) -> felt { alloc_locals;
remove spaces
cairo_programs/stwo_exclusive_programs/qm31_opcodes_test.cairo
line 118 at r7 (raw file):
assert flags[1] = 1; // flag_op0_base_fp } assert flags[3] = 2 - is_imm - flags[0] - flags[1]; // flag_op1_base_fp
(2 - flags[0] - flags[1]) * (1 - is_imm)
cairo_programs/stwo_exclusive_programs/qm31_opcodes_test.cairo
line 176 at r7 (raw file):
} if (is_mul == TRUE) { dw 0x1c05380007ffd7ffc;
missing two asserts
7925df2
to
5c6cd9b
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: all files reviewed, 8 unresolved discussions (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, and @yuvalsw)
cairo_programs/stwo_exclusive_programs/qm31_opcodes_test.cairo
line 80 at r7 (raw file):
Previously, DavidLevitGurevich wrote…
remove spaces
Done.
cairo_programs/stwo_exclusive_programs/qm31_opcodes_test.cairo
line 118 at r7 (raw file):
Previously, DavidLevitGurevich wrote…
(2 - flags[0] - flags[1]) * (1 - is_imm)
Done.
cairo_programs/stwo_exclusive_programs/qm31_opcodes_test.cairo
line 176 at r7 (raw file):
Previously, DavidLevitGurevich wrote…
missing two asserts
Done.
vm/src/typed_operations.rs
line 22 at r7 (raw file):
Previously, DavidLevitGurevich wrote…
why not just
OpcodeExtension::Stone => x.add(y),
because it returns Result(MaybeRelocatable, MathError)
instead of Result(MaybeRelocatable, VirtualMachineError)
so the compiler needs it to be wrapped this way.
vm/src/vm/vm_core.rs
line 2616 at r7 (raw file):
Previously, DavidLevitGurevich wrote…
we need more cases covered
added more tests, let me know whether they are enough
vm/src/vm/decoding/decoder.rs
line 128 at r7 (raw file):
Previously, DavidLevitGurevich wrote…
please change to
..._valid
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 4 of 4 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, and @yuvalsw)
QM31Operations
Description
add packed reduced qm31 add, mul, sub, div via OpcodeExtension::QM31Operations.
Description of the pull request changes and motivation.
Checklist
This change is