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 LogupOps for SIMD backend #641

Open
wants to merge 1 commit into
base: 05-24-Implement_GrandProductOps_for_SIMD_backend
Choose a base branch
from

Conversation

andrewmilson
Copy link
Contributor

@andrewmilson andrewmilson commented May 25, 2024

This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented May 25, 2024

Codecov Report

Attention: Patch coverage is 98.75000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 93.10%. Comparing base (b2e7df0) to head (eda03f0).

Files Patch % Lines
crates/prover/src/core/backend/simd/lookups/gkr.rs 99.21% 0 Missing and 3 partials ⚠️
crates/prover/src/core/lookups/utils.rs 85.71% 2 Missing ⚠️
Additional details and impacted files
@@                                 Coverage Diff                                  @@
##           05-24-Implement_GrandProductOps_for_SIMD_backend     #641      +/-   ##
====================================================================================
+ Coverage                                             92.69%   93.10%   +0.40%     
====================================================================================
  Files                                                    77       77              
  Lines                                                 10487    10847     +360     
  Branches                                              10487    10847     +360     
====================================================================================
+ Hits                                                   9721    10099     +378     
+ Misses                                                  674      653      -21     
- Partials                                                 92       95       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrewmilson andrewmilson force-pushed the 05-24-Implement_GrandProductOps_for_SIMD_backend branch from e17f7b8 to 7ae75ab Compare June 13, 2024 03:40
@andrewmilson andrewmilson force-pushed the 05-24-Implement_LogupOps_for_SIMD_backend branch from 0bfd322 to 9bc0cd3 Compare June 13, 2024 03:40
@andrewmilson andrewmilson force-pushed the 05-24-Implement_GrandProductOps_for_SIMD_backend branch from 7ae75ab to c7b643e Compare June 13, 2024 04:09
@andrewmilson andrewmilson force-pushed the 05-24-Implement_LogupOps_for_SIMD_backend branch from 9bc0cd3 to f81829a Compare June 13, 2024 04:09
@andrewmilson andrewmilson force-pushed the 05-24-Implement_GrandProductOps_for_SIMD_backend branch from c7b643e to 1910db0 Compare June 13, 2024 15:36
@andrewmilson andrewmilson force-pushed the 05-24-Implement_LogupOps_for_SIMD_backend branch from f81829a to 307f62d Compare June 13, 2024 15:37
@andrewmilson andrewmilson force-pushed the 05-24-Implement_GrandProductOps_for_SIMD_backend branch from 1910db0 to 0757511 Compare June 13, 2024 15:46
@andrewmilson andrewmilson force-pushed the 05-24-Implement_LogupOps_for_SIMD_backend branch from 307f62d to 938026c Compare June 13, 2024 15:46
@andrewmilson andrewmilson force-pushed the 05-24-Implement_GrandProductOps_for_SIMD_backend branch from 0757511 to 953ed1c Compare June 19, 2024 03:08
@andrewmilson andrewmilson force-pushed the 05-24-Implement_LogupOps_for_SIMD_backend branch from 938026c to 02de944 Compare June 19, 2024 03:09
@andrewmilson andrewmilson force-pushed the 05-24-Implement_GrandProductOps_for_SIMD_backend branch from 953ed1c to d7bd619 Compare June 19, 2024 03:11
@andrewmilson andrewmilson force-pushed the 05-24-Implement_LogupOps_for_SIMD_backend branch from 02de944 to eaf89a7 Compare June 19, 2024 03:11
@andrewmilson andrewmilson force-pushed the 05-24-Implement_GrandProductOps_for_SIMD_backend branch from d7bd619 to 5e2a76a Compare June 20, 2024 15:13
@andrewmilson andrewmilson force-pushed the 05-24-Implement_LogupOps_for_SIMD_backend branch from eaf89a7 to a418265 Compare June 20, 2024 15:13
@andrewmilson andrewmilson force-pushed the 05-24-Implement_GrandProductOps_for_SIMD_backend branch from 5e2a76a to 98723a9 Compare June 22, 2024 19:05
@andrewmilson andrewmilson force-pushed the 05-24-Implement_LogupOps_for_SIMD_backend branch from a418265 to 9cf2f96 Compare June 22, 2024 19:06
@andrewmilson andrewmilson force-pushed the 05-24-Implement_GrandProductOps_for_SIMD_backend branch from 98723a9 to a2588b0 Compare June 22, 2024 19:10
@andrewmilson andrewmilson force-pushed the 05-24-Implement_LogupOps_for_SIMD_backend branch from 9cf2f96 to 1137271 Compare June 22, 2024 19:10
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.

Reviewed 3 of 4 files at r1, all commit messages.
Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @andrewmilson)


crates/prover/src/core/backend/simd/lookups/gkr.rs line 211 at r1 (raw file):

}

// TODO: Code duplication of `next_logup_generic_layer`. Consider unifying these.

Don't you want to do that in this pr?

Code quote:

// TODO: Code duplication of `next_logup_generic_layer`. Consider unifying these.

crates/prover/src/core/lookups/utils.rs line 199 at r1 (raw file):

/// Projective fraction.
#[derive(Debug, Clone, Copy)]
pub struct Fraction<N, D> {

Why do you need it to be that generic?
Isn't it only for Field/ExtensionField elements?

Code quote:

Fraction<N, D> 

crates/prover/src/core/lookups/utils.rs line 272 at r1 (raw file):

            denominator: self.x * rhs.x,
        }
    }

Can you add a test for this?

Code quote:

    fn add(self, rhs: Self) -> Fraction<T, T> {
        // `1/a + 1/b = (a + b)/(a * b)`
        Fraction {
            numerator: self.x + rhs.x,
            denominator: self.x * rhs.x,
        }
    }

@andrewmilson andrewmilson force-pushed the 05-24-Implement_GrandProductOps_for_SIMD_backend branch from a2588b0 to 5a31462 Compare June 25, 2024 02:11
@andrewmilson andrewmilson force-pushed the 05-24-Implement_LogupOps_for_SIMD_backend branch from 1137271 to ae6f177 Compare June 25, 2024 02:11
@andrewmilson andrewmilson force-pushed the 05-24-Implement_GrandProductOps_for_SIMD_backend branch from 5a31462 to 49955d1 Compare June 25, 2024 02:21
@andrewmilson andrewmilson force-pushed the 05-24-Implement_LogupOps_for_SIMD_backend branch from ae6f177 to 6dcc038 Compare June 25, 2024 02:21
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 4 files reviewed, 3 unresolved discussions (waiting on @shaharsamocha7)


crates/prover/src/core/backend/simd/lookups/gkr.rs line 211 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Don't you want to do that in this pr?

I'd want to just don't think we have the traits in order to express this function as generic


crates/prover/src/core/lookups/utils.rs line 199 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Why do you need it to be that generic?
Isn't it only for Field/ExtensionField elements?

We don't implement Field or ExtensionOf traits on packed field elements.
If we had abstract Field/Extension traits it would work

@andrewmilson andrewmilson force-pushed the 05-24-Implement_GrandProductOps_for_SIMD_backend branch from 49955d1 to b2e7df0 Compare June 26, 2024 14:34
@andrewmilson andrewmilson force-pushed the 05-24-Implement_LogupOps_for_SIMD_backend branch from 6dcc038 to eda03f0 Compare June 26, 2024 14:34
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

3 participants