Skip to content
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

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

andrewmilson
Copy link
Contributor

@andrewmilson andrewmilson commented May 16, 2024

This change is Reviewable

@andrewmilson andrewmilson force-pushed the 05-08-Add_GKR_implementation_of_Logup_lookups branch from 53e9d4e to ccbe3b0 Compare June 25, 2024 02:20
@andrewmilson andrewmilson force-pushed the 05-15-Implement_MleOps_for_SIMD_backend branch from 8ceac89 to e175251 Compare June 25, 2024 02:21
Copy link
Collaborator

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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: 

@andrewmilson andrewmilson force-pushed the 05-08-Add_GKR_implementation_of_Logup_lookups branch from ccbe3b0 to 1945331 Compare June 26, 2024 14:33
@andrewmilson andrewmilson force-pushed the 05-15-Implement_MleOps_for_SIMD_backend branch from e175251 to c51f096 Compare June 26, 2024 14:33
@andrewmilson andrewmilson force-pushed the 05-08-Add_GKR_implementation_of_Logup_lookups branch from 1945331 to 47caaef Compare July 11, 2024 14:05
@andrewmilson andrewmilson force-pushed the 05-15-Implement_MleOps_for_SIMD_backend branch from c51f096 to d351947 Compare July 11, 2024 14:05
@andrewmilson andrewmilson force-pushed the 05-08-Add_GKR_implementation_of_Logup_lookups branch from 47caaef to ed6103f Compare July 11, 2024 14:35
@andrewmilson andrewmilson force-pushed the 05-15-Implement_MleOps_for_SIMD_backend branch from d351947 to ef6c4aa Compare July 11, 2024 14:36
@andrewmilson andrewmilson force-pushed the 05-08-Add_GKR_implementation_of_Logup_lookups branch from ed6103f to d9935c0 Compare July 11, 2024 14:40
@andrewmilson andrewmilson force-pushed the 05-15-Implement_MleOps_for_SIMD_backend branch from ef6c4aa to 89a31c7 Compare July 11, 2024 14:40
Copy link
Collaborator

@spapinistarkware spapinistarkware left a 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.

Copy link
Collaborator

@spapinistarkware spapinistarkware left a 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

Copy link
Collaborator

@spapinistarkware spapinistarkware left a 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)

@andrewmilson andrewmilson force-pushed the 05-08-Add_GKR_implementation_of_Logup_lookups branch from d9935c0 to c137974 Compare July 15, 2024 13:02
@andrewmilson andrewmilson force-pushed the 05-15-Implement_MleOps_for_SIMD_backend branch from 89a31c7 to 6b5a7f9 Compare July 15, 2024 13:02
@andrewmilson andrewmilson force-pushed the 05-08-Add_GKR_implementation_of_Logup_lookups branch from c137974 to 3da51dc Compare July 16, 2024 17:07
Base automatically changed from 05-08-Add_GKR_implementation_of_Logup_lookups to dev July 16, 2024 17:13
@andrewmilson andrewmilson force-pushed the 05-15-Implement_MleOps_for_SIMD_backend branch from 6b5a7f9 to cc39381 Compare July 16, 2024 18:43
Copy link
Contributor Author

@andrewmilson andrewmilson left a 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 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.

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.

Copy link
Collaborator

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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'

@andrewmilson andrewmilson force-pushed the 05-15-Implement_MleOps_for_SIMD_backend branch from cc39381 to 967b6f3 Compare July 17, 2024 14:12
Copy link
Contributor Author

@andrewmilson andrewmilson left a 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.

@andrewmilson andrewmilson force-pushed the 05-15-Implement_MleOps_for_SIMD_backend branch from 967b6f3 to 78bd15b Compare July 17, 2024 14:37
@andrewmilson andrewmilson merged commit a253a49 into dev Jul 17, 2024
13 of 14 checks passed
@andrewmilson andrewmilson deleted the 05-15-Implement_MleOps_for_SIMD_backend branch July 17, 2024 14:42
This was referenced Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants