-
Notifications
You must be signed in to change notification settings - Fork 89
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
Implement MleOps for SIMD backend #629
Conversation
53e9d4e
to
ccbe3b0
Compare
8ceac89
to
e175251
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 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @andrewmilson)
crates/prover/src/core/backend/simd/lookups/mle.rs
line 121 at r2 (raw file):
/// Computes all `eq(0, assignment_i) * lhs_eval_i + eq(1, assignment_i) * rhs_eval_i`. // TODO: Consider unifying fold_packed_mle_* functions once we have something like
Please put your name in the todo's. can be fixed later
Code quote:
TODO:
ccbe3b0
to
1945331
Compare
e175251
to
c51f096
Compare
1945331
to
47caaef
Compare
c51f096
to
d351947
Compare
47caaef
to
ed6103f
Compare
d351947
to
ef6c4aa
Compare
ed6103f
to
d9935c0
Compare
ef6c4aa
to
89a31c7
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 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @andrewmilson)
crates/prover/src/core/backend/simd/lookups/mle.rs
line 23 at r2 (raw file):
let midpoint = mle.len() / 2; // Use CPU backend to to prevent dealing with instances smaller than `PackedSecureField`.
Suggestion:
// Use CPU backend to to avoid dealing with instances smaller than `PackedSecureField`.
crates/prover/src/core/backend/simd/lookups/mle.rs
line 44 at r2 (raw file):
} impl MleOps<SecureField> for SimdBackend {
Looks like exactly the same code. Can you unite by negin generic on F and EF, and requiring some ops?
Such as F: Mul<SecureField, Output=EF>
I head a similar issue at crates/prover/src/constraint_framework/mod.rs
. Currently have a todo to use a better trait, so you can copy that TODO here as well, in addition.
Code quote:
impl MleOps<SecureField> for SimdBackend {
crates/prover/src/core/backend/simd/lookups/mle.rs
line 78 at r2 (raw file):
} impl MultivariatePolyOracle for Mle<SimdBackend, SecureField> {
Do we actually need this?
It seems a bit wierd, since you're trying to represent this Mle as a multilinear polynomial of it self. Why would we ever require this?
Seems to me that when we get a query to the first layer, the Mle, we can just answer the query directly using the reduction.
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, 5 unresolved discussions (waiting on @andrewmilson)
crates/prover/src/core/backend/simd/lookups/mle.rs
line 123 at r2 (raw file):
// TODO: Consider unifying fold_packed_mle_* functions once we have something like // AbstractField/AbstractExtensionField traits implemented on packed types. fn fold_packed_mle_secure_evals(
Same as above, this can be generic on F and EF
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, 1 of 1 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @andrewmilson)
d9935c0
to
c137974
Compare
89a31c7
to
6b5a7f9
Compare
c137974
to
3da51dc
Compare
6b5a7f9
to
cc39381
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 3 files reviewed, 4 unresolved discussions (waiting on @shaharsamocha7 and @spapinistarkware)
crates/prover/src/core/backend/simd/lookups/mle.rs
line 23 at r2 (raw file):
let midpoint = mle.len() / 2; // Use CPU backend to to prevent dealing with instances smaller than `PackedSecureField`.
Done.
crates/prover/src/core/backend/simd/lookups/mle.rs
line 44 at r2 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
Looks like exactly the same code. Can you unite by negin generic on F and EF, and requiring some ops?
Such as F: Mul<SecureField, Output=EF>
I head a similar issue atcrates/prover/src/constraint_framework/mod.rs
. Currently have a todo to use a better trait, so you can copy that TODO here as well, in addition.
This implementation modifies in place but the other makes an allocation which makes it hard to unify.
crates/prover/src/core/backend/simd/lookups/mle.rs
line 78 at r2 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
Do we actually need this?
It seems a bit wierd, since you're trying to represent this Mle as a multilinear polynomial of it self. Why would we ever require this?
Seems to me that when we get a query to the first layer, the Mle, we can just answer the query directly using the reduction.
Done. Removed
crates/prover/src/core/backend/simd/lookups/mle.rs
line 121 at r2 (raw file):
Previously, shaharsamocha7 wrote…
Please put your name in the todo's. can be fixed later
Done.
crates/prover/src/core/backend/simd/lookups/mle.rs
line 123 at r2 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
Same as above, this can be generic on F and EF
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 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @andrewmilson)
crates/prover/src/core/backend/simd/lookups/mle.rs
line 23 at r2 (raw file):
Previously, andrewmilson (Andrew Milson) wrote…
Done.
Remove extra 'to'
cc39381
to
967b6f3
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, 1 unresolved discussion (waiting on @spapinistarkware)
crates/prover/src/core/backend/simd/lookups/mle.rs
line 23 at r2 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
Remove extra 'to'
Done.
967b6f3
to
78bd15b
Compare
This change is