Implement QM31 libfuncs #1429
Implement QM31 libfuncs #1429
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1429 +/- ##
==========================================
+ Coverage 81.45% 81.93% +0.48%
==========================================
Files 105 107 +2
Lines 25862 26598 +736
==========================================
+ Hits 21065 21794 +729
- Misses 4797 4804 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Benchmark results Main vs HEAD.Base
Head
Base
Head
Base
Head
Base
Head
Base
Head
Base
Head
Base
Head
|
Benchmarking resultsBenchmark for program
|
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
11.014 ± 0.039 | 10.956 | 11.083 | 4.29 ± 0.05 |
cairo-native (embedded AOT) |
2.569 ± 0.032 | 2.534 | 2.625 | 1.00 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
2.662 ± 0.020 | 2.630 | 2.702 | 1.04 ± 0.02 |
Benchmark for program dict_snapshot
Open benchmarks
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
550.4 ± 4.4 | 545.9 | 558.1 | 1.00 |
cairo-native (embedded AOT) |
2310.0 ± 26.3 | 2280.5 | 2365.2 | 4.20 ± 0.06 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
2424.7 ± 21.9 | 2389.8 | 2454.6 | 4.41 ± 0.05 |
Benchmark for program factorial_2M
Open benchmarks
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
4.822 ± 0.020 | 4.804 | 4.869 | 1.79 ± 0.02 |
cairo-native (embedded AOT) |
2.700 ± 0.025 | 2.674 | 2.764 | 1.00 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
2.718 ± 0.015 | 2.692 | 2.738 | 1.01 ± 0.01 |
Benchmark for program fib_2M
Open benchmarks
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
4.727 ± 0.028 | 4.682 | 4.768 | 2.08 ± 0.02 |
cairo-native (embedded AOT) |
2.275 ± 0.021 | 2.232 | 2.306 | 1.00 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
2.312 ± 0.026 | 2.260 | 2.359 | 1.02 ± 0.01 |
Benchmark for program heavy_circuit
Open benchmarks
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
11.014 ± 0.124 | 10.807 | 11.261 | 1.01 ± 0.02 |
cairo-native (embedded AOT) |
10.926 ± 0.198 | 10.624 | 11.181 | 1.00 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
11.131 ± 0.161 | 10.921 | 11.401 | 1.02 ± 0.02 |
Benchmark for program linear_search
Open benchmarks
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
575.1 ± 4.9 | 568.7 | 581.8 | 1.00 |
cairo-native (embedded AOT) |
2345.9 ± 15.4 | 2324.7 | 2382.1 | 4.08 ± 0.04 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
2482.3 ± 18.1 | 2459.1 | 2516.2 | 4.32 ± 0.05 |
Benchmark for program logistic_map
Open benchmarks
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
395.0 ± 5.6 | 387.4 | 407.8 | 1.00 |
cairo-native (embedded AOT) |
2449.6 ± 27.2 | 2409.6 | 2493.1 | 6.20 ± 0.11 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
2652.4 ± 24.0 | 2605.7 | 2678.9 | 6.72 ± 0.11 |
src/metadata/runtime_bindings.rs
Outdated
| /// Operation is done between `lhs_ptr` and `rhs_ptr` while the result is stored | ||
| /// in `res_ptr`. |
There was a problem hiding this comment.
This comment is outdated, right?
Also, I would mention that lhs_ptr and rhs_ptr are pointers to a value of type QM31
There was a problem hiding this comment.
Mmm I wouldn't say outdated, the operation is still using those pointers and the result is being stored in the res_ptr which is now created in the function and is not a parameter. I added the mention about the pointers here b7b0bd0
| #[cfg(target_arch = "x86_64")] | ||
| return Err(Error::ParseAttributeError); |
There was a problem hiding this comment.
I would add a comment with something like: "on x86_64, a QM31 value is always returned by memory, therefore making this branch is unreachable on that architecture".
There was a problem hiding this comment.
A QM31 is not bigger than an u128, but its returned as a pointer regardless. I would remove that part of the comment as its probably more complex than that.
src/runtime.rs
Outdated
|
|
||
| // SAFETY: The only possible error is if rhs is zero. However, in the QM31 division libfunc, the divisor | ||
| // is of type NonZero<qm31> which ensures that we are not falling into the error case. | ||
| *res = to_representative_coefficients((lhs / rhs).unwrap()); |
There was a problem hiding this comment.
Can you use expect instead of unwrap?
JulianGCalderon
left a comment
JulianGCalderon
left a comment
There was a problem hiding this comment.
We should reenable the corelib QM31 tests. See https://github.com/lambdaclass/cairo_native/blob/35361e287744ce95abf4528da5e82c24fc8d261b/corelib.patch
| // TODO: Refactor cairo functions to receive m31 as parameters so we don't need different ones | ||
| // to test different cases and we can unify them into one. This can be done when issue #1217 gets closed. |
There was a problem hiding this comment.
BoundedInt case is not implemented in to_ptr() method so we can pass it as an argument from Rust to Cairo. I didn´t implement it here because it is out of the scope of this PR
Co-authored-by: Gabriel Bosio <[email protected]>
JulianGCalderon
left a comment
JulianGCalderon
left a comment
There was a problem hiding this comment.
Left some nits, but looks good.
Given that this PR requires the EGCD algorithm, perhaps its better to join the implementations first, and then merge this PR reusing the existing implementations (so that we don't duplicate the implementation again)
src/metadata/runtime_bindings.rs
Outdated
| /// Executes the operation on the `QM31` values referenced by `lhs_ptr` and `rhs_ptr`, | ||
| /// and stores the resulting `QM31` in `res_ptr`. |
There was a problem hiding this comment.
I wouldn't mention res_ptr, its an implementation detail. The user should only care that this function takes two pointers to an QM31, and returns a QM31 value.
src/metadata/runtime_bindings.rs
Outdated
| /// Executes the operation on the `QM31` values referenced by `lhs_ptr` and `rhs_ptr`, | ||
| /// and stores the resulting `QM31` in `res_ptr`. | ||
| /// | ||
| /// Returns a opaque pointer as the result. |
There was a problem hiding this comment.
It doesn't return an opaque pointer, but a QM31 value, because of the load at line 622
| #[cfg(target_arch = "x86_64")] | ||
| return Err(Error::ParseAttributeError); |
There was a problem hiding this comment.
A QM31 is not bigger than an u128, but its returned as a pointer regardless. I would remove that part of the comment as its probably more complex than that.
Agree |
Implement QM31 libfuncs
Implements all the QM31 related libfuncs:
Also added some tests and I was able to run the QM31 tests from the corelib:
% cargo run -p cairo-native-test corelib/src/test/qm31_test.cairo -s Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.27s Running `target/debug/cairo-native-test corelib/src/test/qm31_test.cairo -s` running 6 tests test qm31_test::qm31_test::test_qm31_add_and_sub ... ok (gas usage est.: 2100) test qm31_test::qm31_test::test_qm31_mul_and_div ... ok (gas usage est.: 2500) test qm31_test::qm31_test::test_qm31_inverse ... ok (gas usage est.: 2600) test qm31_test::qm31_test::test_pack ... ok (gas usage est.: 5700) test qm31_test::qm31_test::test_unpack ... ok (gas usage est.: 14800) test qm31_test::qm31_test::test_m31_into_qm31 ... ok (gas usage est.: 13600) test result: ok. 6 passed; 0 failed; 0 ignored; 0 filtered out;Closes #1425
Introduces Breaking Changes?
Yes/No.
starknet-blocks.ymlworkflow to use these PRs.These PRs should be merged after this one right away, in that order.
Checklist