o1vm/mips: fix failing constraints #2729
Conversation
marcbeunardeau88
force-pushed
the
marc/fix-vm
branch
from
85a3b48 to
f640337
Compare
|
Can you rebase on top of #2700 for the CI please? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## test/o1vm-e2e #2729 +/- ##
==============================================
Coverage 72.51% 72.51%
==============================================
Files 247 247
Lines 57705 57745 +40
==============================================
+ Hits 41845 41875 +30
- Misses 15860 15870 +10 ☔ View full report in Codecov by Sentry. |
marcbeunardeau88
force-pushed
the
marc/fix-vm
branch
from
f640337 to
f0aa273
Compare
There was a problem hiding this comment.
I left a few suggestions to help keep the different interpreters separated in scope, I hope it can be useful. Other than that, I will happily approve the PR, as it does not change all of the Self::Variables to field, but it keeps it as native type. Either way, I believe that addressing Danny's optimization can be a smart move after the constraints fix is merged. But I would first focus on this PR and work on #2040 / and #2047 afterwards, because it is a different issue being targeted there.
| }; | ||
| let _to_zero_test_inv_or_zero = { | ||
| let pos = self.alloc_scratch(); | ||
| let inv_or_zero = if to_zero_test == Fp::zero() { |
There was a problem hiding this comment.
You can probably call inverse_or_zero in these lines, no? I guess the bug comes from the res block, there's no need to unroll the inv_or_zero to solve the bug.
There was a problem hiding this comment.
I thought so at first as well, but Inverse_or_zero is forced to return a Variable by the signature, so we can't get the resulting field_inversion we desire here.
| } else { | ||
| self.is_zero(&(*y - *x)) | ||
| } | ||
| // We replicate is_zero(x-y), but working on field elt, |
There was a problem hiding this comment.
Perhaps it's less error prone if you move the is_zero() in interpreter.rs into constraint.rs and define a variant of is_zero() in witness.rs containing precisely your replication here in equal(). Otherwise, one could call the is_zero() from interpreter.rs for the witness and incur in the same problem being fixed in this PR.
There was a problem hiding this comment.
Indeed it is more clean
|
@marcbeunardeau88 can you change the target branch to match @shimkiv one please? |
| } | ||
| // We replicate is_zero(x-y), but working on field elt, | ||
| // to avoid subtraction overflow in the witness interpreter for u32 | ||
| let to_zero_test = Fp::from(*x) - Fp::from(*y); |
There was a problem hiding this comment.
You don't need to convert as early as here in Fp.
There was a problem hiding this comment.
Indeed, but I believe this to be more clear, and the performance is negligibly impacted
|
Please reopen the PR targetting master, @marcbeunardeau88 |
fixe the remaining failing Mips instr
see : #2075
Closes #2075