- 
                Notifications
    You must be signed in to change notification settings 
- Fork 49
Improve QutientConstantsImpl::gen performance (mostly in naive impl) #1305
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
base: main
Are you sure you want to change the base?
Conversation
c38af99    to
    6eef425      
    Compare
  
    | Can you move this to sharp7 and document? | 
| 
 
 Was there an inverse here before? Code quote:  let im_py_inv = CM31Trait::pack(c, d).inverse(); | 
| I guess we need to decide if we want it in the mvp | 
| 
 
 This shouldn't be included through this PR. Code quote:   let minus_quotient = minus_numerator.reduce().mul_cm31(denom_inv); | 
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: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)
stwo_cairo_verifier/crates/verifier_core/src/pcs/quotients.cairo line 216 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
This shouldn't be included through this PR.
I know, but without it I had to inroduce ::zero which was an unnecessary complication. The branch over sharp 7 ( #1332 ) has this previously introduced.
stwo_cairo_verifier/crates/verifier_core/src/pcs/quotients.cairo line 314 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
Was there an inverse here before?
No. Previously every iteration multiplied alpha by the constant neg_dbl_im_py, which was very wasteful.
Instead, in this version we initialize alpha to that value, so everything is inherently multiplied by it. (Theoretically this is is unnecessary, the constant can be taken out at the protocol level. But this would require a change in the prover as well.)
Because we have to return alpha, we must divided by the same term in the end. Also some of ther other terms need to be divided by it.
Possibly there is a more efficient way to do this without the divisions (depending on how much more expensive they are than a mul).
We can also probably batch these, although that would be somewhat annoying.
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.
@ilyalesokhin-starkware reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 2 of 3 files reviewed, all discussions resolved
stwo_cairo_verifier/crates/verifier_core/src/pcs/quotients.cairo line 314 at r1 (raw file):
Previously, dancarmoz (Dan Carmon) wrote…
No. Previously every iteration multiplied alpha by the constant
neg_dbl_im_py, which was very wasteful.
Instead, in this version we initialize alpha to that value, so everything is inherently multiplied by it. (Theoretically this is is unnecessary, the constant can be taken out at the protocol level. But this would require a change in the prover as well.)
Because we have to return alpha, we must divided by the same term in the end. Also some of ther other terms need to be divided by it.
Possibly there is a more efficient way to do this without the divisions (depending on how much more expensive they are than a mul).
We can also probably batch these, although that would be somewhat annoying.
I think we should make the prover change as well in that case, and do it on main
This change is