Skip to content

Conversation

Gali-StarkWare
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

Gali-StarkWare commented Apr 29, 2025

Copy link
Contributor

@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.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion


crates/prover/src/core/poly/circle/evaluation.rs line 249 at r1 (raw file):

// TODO(Gali): Remove.
#[allow(dead_code)]
fn simd_weights(

This function should have unit test

Code quote:

fn simd_weights(

@Gali-StarkWare Gali-StarkWare changed the base branch from gali/cpu_barycentric to graphite-base/1069 May 14, 2025 07:23
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: c76f10e Previous: 615de1f Ratio
iffts/simd ifft/27 588941361 ns/iter (± 7171756) 277997930 ns/iter (± 8211035) 2.12
iffts/simd ifft/28 1209210657 ns/iter (± 15563836) 574305772 ns/iter (± 32121353) 2.11

This comment was automatically generated by workflow using github-action-benchmark.

CC: @shaharsamocha7

@Gali-StarkWare Gali-StarkWare force-pushed the gali/simd_barycentric branch from baac125 to ce3c080 Compare May 14, 2025 07:47
@Gali-StarkWare Gali-StarkWare changed the base branch from graphite-base/1069 to gali/cpu_barycentric May 14, 2025 07:47
@codecov-commenter
Copy link

codecov-commenter commented May 14, 2025

Codecov Report

Attention: Patch coverage is 98.38710% with 2 lines in your changes missing coverage. Please review.

Project coverage is 92.73%. Comparing base (ad1902f) to head (c5056c4).
Report is 1 commits behind head on gali/barycentric.

Files with missing lines Patch % Lines
crates/prover/src/core/poly/circle/evaluation.rs 98.38% 2 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                  @@
##           gali/barycentric    #1069      +/-   ##
====================================================
+ Coverage             92.68%   92.73%   +0.04%     
====================================================
  Files                   106      106              
  Lines                 14374    14497     +123     
  Branches              14374    14497     +123     
====================================================
+ Hits                  13323    13444     +121     
- Misses                  970      972       +2     
  Partials                 81       81              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Gali-StarkWare Gali-StarkWare changed the base branch from gali/cpu_barycentric to graphite-base/1069 May 14, 2025 08:03
@Gali-StarkWare Gali-StarkWare changed the base branch from graphite-base/1069 to gali/cpu_barycentric May 25, 2025 14:20
@Gali-StarkWare Gali-StarkWare changed the base branch from gali/cpu_barycentric to graphite-base/1069 May 25, 2025 14:23
@Gali-StarkWare Gali-StarkWare changed the base branch from graphite-base/1069 to gali/cpu_barycentric May 25, 2025 14:41
@Gali-StarkWare Gali-StarkWare changed the base branch from gali/cpu_barycentric to graphite-base/1069 May 28, 2025 07:19
@Gali-StarkWare Gali-StarkWare force-pushed the gali/simd_barycentric branch from ce3c080 to 986c607 Compare May 28, 2025 08:49
@Gali-StarkWare Gali-StarkWare changed the base branch from graphite-base/1069 to gali/cpu_barycentric May 28, 2025 08:49
@Gali-StarkWare Gali-StarkWare marked this pull request as ready for review May 28, 2025 09:25
@Gali-StarkWare Gali-StarkWare changed the base branch from gali/cpu_barycentric to graphite-base/1069 May 28, 2025 09:25
@Gali-StarkWare Gali-StarkWare force-pushed the gali/simd_barycentric branch from 986c607 to eda6d66 Compare May 29, 2025 13:17
@Gali-StarkWare Gali-StarkWare changed the base branch from graphite-base/1069 to gali/cpu_barycentric May 29, 2025 13:17
@Gali-StarkWare Gali-StarkWare force-pushed the gali/simd_barycentric branch 2 times, most recently from b901e8e to 886ec2e Compare May 29, 2025 13:28
@Gali-StarkWare Gali-StarkWare changed the base branch from gali/cpu_barycentric to graphite-base/1069 June 5, 2025 10:27
@Gali-StarkWare Gali-StarkWare force-pushed the gali/simd_barycentric branch from 886ec2e to 8218dfb Compare June 5, 2025 10:27
@graphite-app graphite-app bot changed the base branch from graphite-base/1069 to gali/barycentric June 5, 2025 10:28
@Gali-StarkWare Gali-StarkWare force-pushed the gali/simd_barycentric branch from 8218dfb to faab86f Compare June 5, 2025 10:28
Copy link
Contributor

@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 all commit messages.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @Gali-StarkWare)


crates/prover/src/core/poly/circle/evaluation.rs line 249 at r1 (raw file):

Previously, shaharsamocha7 wrote…

This function should have unit test

I want a test that compares this with the cpu weights
Also, try to simplify the code as we did in the cpu pr


crates/prover/src/core/poly/circle/evaluation.rs line 321 at r4 (raw file):

    weights: &Col<SimdBackend, SecureField>,
) -> SecureField {
    let evals = evals.clone().bit_reverse();

Is this going to be removed?
If so, please have a TODO on that

Code quote:

let evals = evals.clone().bit_reverse();

crates/prover/src/core/poly/circle/evaluation.rs line 326 at r4 (raw file):

            acc + (weights.at(i) * evals.values.at(i))
        });
    }

could we discard this usecase?

Code quote:

    if evals.domain.size() < N_LANES {
        return (0..evals.domain.size()).fold(SecureField::zero(), |acc, i| {
            acc + (weights.at(i) * evals.values.at(i))
        });
    }

Copy link
Contributor Author

@Gali-StarkWare Gali-StarkWare 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: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @Gali-StarkWare and @shaharsamocha7)


crates/prover/src/core/poly/circle/evaluation.rs line 249 at r1 (raw file):

Previously, shaharsamocha7 wrote…

I want a test that compares this with the cpu weights
Also, try to simplify the code as we did in the cpu pr

Added the test. Do you want to split the si_i optimization (using only 2 values because it alternates) to another pr?


crates/prover/src/core/poly/circle/evaluation.rs line 321 at r4 (raw file):

Previously, shaharsamocha7 wrote…

Is this going to be removed?
If so, please have a TODO on that

I have a TODO in the weights function to change the order, do we need another one here? Same for the CPU function?


crates/prover/src/core/poly/circle/evaluation.rs line 326 at r4 (raw file):

Previously, shaharsamocha7 wrote…

could we discard this usecase?

Done.

@Gali-StarkWare Gali-StarkWare force-pushed the gali/simd_barycentric branch from 209a21d to c76f10e Compare June 5, 2025 13:19
Copy link
Contributor

@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 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Gali-StarkWare)


crates/prover/src/core/poly/circle/evaluation.rs line 238 at r6 (raw file):

/// Computes the weights for Barycentric Lagrange interpolation for point `p` on `coset`.
/// `p` must not be in the domain. For more information, see [`barycentric_weights`].
fn simd_barycentric_weights(

Shouldn't this code be under simd folder somewhere?
I'm not sure

Code quote:

fn simd_barycentric_weights(

crates/prover/src/core/poly/circle/evaluation.rs line 281 at r6 (raw file):

    // TODO(Gali): Change weights order to bit-reverse order.
    // S_i(i) is invariant under G_(n−1) and alternate under J

Can you clarify this comment?
It should explain why the second part is multiplied by -1, but I think that comment is a bit implicit.

Code quote:

  // S_i(i) is invariant under G_(n−1) and alternate under J

crates/prover/src/core/poly/circle/evaluation.rs line 292 at r6 (raw file):

        .collect();

    weights

Does this pass compiler?

Suggestion:

    (0..weights_vec_len)
        .map(|i| {
            if i < weights_vec_len / 2 {
                vi_p_inverse[i] * si_0_vn_p
            } else {
                vi_p_inverse[i] * -si_0_vn_p
            }
        })
        .collect()

Copy link
Contributor Author

@Gali-StarkWare Gali-StarkWare 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: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @shaharsamocha7)


crates/prover/src/core/poly/circle/evaluation.rs line 238 at r6 (raw file):

Previously, shaharsamocha7 wrote…

Shouldn't this code be under simd folder somewhere?
I'm not sure

Next pr is moving it to simd


crates/prover/src/core/poly/circle/evaluation.rs line 292 at r6 (raw file):

Previously, shaharsamocha7 wrote…

Does this pass compiler?

Yes, it's like that because in a following parallelism pr I need to do it that way. Would you rather I change it to your suggestion and define weights there?

Copy link
Contributor

@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 r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Gali-StarkWare)

Copy link
Contributor Author

@Gali-StarkWare Gali-StarkWare 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 1 files at r7.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @shaharsamocha7)

@Gali-StarkWare Gali-StarkWare force-pushed the gali/simd_barycentric branch from 4681461 to c5056c4 Compare June 9, 2025 07:50
Copy link
Contributor

@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 1 of 1 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Gali-StarkWare)

Copy link
Contributor Author

@Gali-StarkWare Gali-StarkWare left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Gali-StarkWare)

@Gali-StarkWare Gali-StarkWare merged commit 88e1c1c into gali/barycentric Jun 9, 2025
20 checks passed
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.

4 participants