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 FieldOps for SIMD backend #593

Merged
merged 1 commit into from
May 15, 2024

Conversation

andrewmilson
Copy link
Contributor

@andrewmilson andrewmilson commented May 1, 2024

This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented May 1, 2024

Codecov Report

Attention: Patch coverage is 54.36893% with 47 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (04-30-Add_SIMD_backend_arithmetic@d7299c5). Click here to learn what that means.

Files Patch % Lines
crates/prover/src/core/backend/simd/column.rs 54.36% 45 Missing and 2 partials ⚠️
Additional details and impacted files
@@                         Coverage Diff                          @@
##             04-30-Add_SIMD_backend_arithmetic     #593   +/-   ##
====================================================================
  Coverage                                     ?   91.16%           
====================================================================
  Files                                        ?       75           
  Lines                                        ?     9438           
  Branches                                     ?     9438           
====================================================================
  Hits                                         ?     8604           
  Misses                                       ?      762           
  Partials                                     ?       72           

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

@andrewmilson andrewmilson force-pushed the 04-30-Add_SIMD_backend_arithmetic branch from 51d9333 to d2ca30c Compare May 1, 2024 04:39
@andrewmilson andrewmilson force-pushed the 04-30-Implement_FieldOps_for_SIMD_backend branch from 0143448 to 9bd0fc4 Compare May 1, 2024 04:42
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.

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @andrewmilson)


crates/prover/src/core/backend/simd/column.rs line 15 at r1 (raw file):

    pub data: Vec<PackedBaseField>,
    pub length: usize,
}

We already have this struct in avx512.
Can't we reuse it?

Code quote:

#[derive(Clone, Debug)]
pub struct BaseFieldVec {
    pub data: Vec<PackedBaseField>,
    pub length: usize,
}

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: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @shaharsamocha7)


crates/prover/src/core/backend/simd/column.rs line 15 at r1 (raw file):

Previously, shaharsamocha7 wrote…

We already have this struct in avx512.
Can't we reuse it?

Resolved offline.
avx512 is conditionally compiled for x86.

@andrewmilson andrewmilson force-pushed the 04-30-Add_SIMD_backend_arithmetic branch from d2ca30c to c3c6fd2 Compare May 1, 2024 15:51
@andrewmilson andrewmilson force-pushed the 04-30-Implement_FieldOps_for_SIMD_backend branch 2 times, most recently from e9ed60d to aca433d Compare May 1, 2024 15:53
@andrewmilson andrewmilson force-pushed the 04-30-Add_SIMD_backend_arithmetic branch from c3c6fd2 to 7ddb0a1 Compare May 2, 2024 16:29
@andrewmilson andrewmilson force-pushed the 04-30-Implement_FieldOps_for_SIMD_backend branch from aca433d to 6d71f67 Compare May 2, 2024 16:29
@andrewmilson andrewmilson force-pushed the 04-30-Add_SIMD_backend_arithmetic branch from 7ddb0a1 to 2c82ea1 Compare May 2, 2024 16:30
@andrewmilson andrewmilson force-pushed the 04-30-Implement_FieldOps_for_SIMD_backend branch from 6d71f67 to 455686c Compare May 2, 2024 16:30
@andrewmilson andrewmilson force-pushed the 04-30-Add_SIMD_backend_arithmetic branch from 2c82ea1 to d8abf68 Compare May 2, 2024 16:34
@andrewmilson andrewmilson force-pushed the 04-30-Implement_FieldOps_for_SIMD_backend branch from 455686c to ea02309 Compare May 2, 2024 16:34
@andrewmilson andrewmilson force-pushed the 04-30-Add_SIMD_backend_arithmetic branch from d8abf68 to 063029c Compare May 2, 2024 19:23
@andrewmilson andrewmilson force-pushed the 04-30-Implement_FieldOps_for_SIMD_backend branch from ea02309 to 567e20c Compare May 2, 2024 19:23
@andrewmilson andrewmilson force-pushed the 04-30-Add_SIMD_backend_arithmetic branch from 063029c to 2ca8966 Compare May 2, 2024 20:08
@andrewmilson andrewmilson force-pushed the 04-30-Implement_FieldOps_for_SIMD_backend branch from 567e20c to b6daa79 Compare May 2, 2024 20:08
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.

Like I said in rpevious PRs, it would be a lot easier to cehck these PRs if you first copied the avx file in a separate commit. Then we can just check the changes.

Reviewed all commit messages.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @shaharsamocha7)

@andrewmilson andrewmilson force-pushed the 04-30-Add_SIMD_backend_arithmetic branch from b8a5161 to 92fb94e Compare May 6, 2024 19:16
@andrewmilson andrewmilson force-pushed the 04-30-Implement_FieldOps_for_SIMD_backend branch from 0f31f09 to 9509a53 Compare May 6, 2024 19:16
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 2 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @andrewmilson)


crates/prover/src/core/backend/simd/column.rs line 27 at r2 (raw file):

        &mut cast_slice_mut(&mut self.data)[..self.length]
    }
}

We used to have as_slice, as_mut_slice. Is that ok?

Code quote:

impl AsRef<[BaseField]> for BaseFieldVec {
    fn as_ref(&self) -> &[BaseField] {
        &cast_slice(&self.data)[..self.length]
    }
}

impl AsMut<[BaseField]> for BaseFieldVec {
    fn as_mut(&mut self) -> &mut [BaseField] {
        &mut cast_slice_mut(&mut self.data)[..self.length]
    }
}

crates/prover/src/core/backend/simd/mod.rs line 24 at r2 (raw file):

        todo!()
    }
}

Why here and not in column.rs?

Code quote:

impl ColumnOps<BaseField> for SimdBackend {
    type Column = BaseFieldVec;

    fn bit_reverse_column(_column: &mut Self::Column) {
        todo!()
    }
}

@andrewmilson andrewmilson force-pushed the 04-30-Implement_FieldOps_for_SIMD_backend branch from 9509a53 to 8db394e Compare May 7, 2024 18:50
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: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @shaharsamocha7)


crates/prover/src/core/backend/simd/column.rs line 27 at r2 (raw file):

Previously, shaharsamocha7 wrote…

We used to have as_slice, as_mut_slice. Is that ok?

Yeah, it's better. Updated


crates/prover/src/core/backend/simd/mod.rs line 24 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Why here and not in column.rs?

Done.

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

@andrewmilson andrewmilson force-pushed the 04-30-Add_SIMD_backend_arithmetic branch from 92fb94e to a82d991 Compare May 8, 2024 15:45
@andrewmilson andrewmilson force-pushed the 04-30-Implement_FieldOps_for_SIMD_backend branch from 8db394e to 0990999 Compare May 8, 2024 15:45
@andrewmilson andrewmilson force-pushed the 04-30-Add_SIMD_backend_arithmetic branch from a82d991 to d555471 Compare May 10, 2024 21:11
@andrewmilson andrewmilson force-pushed the 04-30-Implement_FieldOps_for_SIMD_backend branch from 0990999 to d79b8b3 Compare May 10, 2024 21:11
@andrewmilson andrewmilson force-pushed the 04-30-Add_SIMD_backend_arithmetic branch from d555471 to 7c485c7 Compare May 11, 2024 02:33
@andrewmilson andrewmilson force-pushed the 04-30-Implement_FieldOps_for_SIMD_backend branch from d79b8b3 to aa87739 Compare May 11, 2024 02:40
@andrewmilson andrewmilson force-pushed the 04-30-Add_SIMD_backend_arithmetic branch from 7c485c7 to d7299c5 Compare May 12, 2024 13:48
@andrewmilson andrewmilson force-pushed the 04-30-Implement_FieldOps_for_SIMD_backend branch from aa87739 to fd674a3 Compare May 15, 2024 04:34
Base automatically changed from 04-30-Add_SIMD_backend_arithmetic to dev May 15, 2024 13:09
@andrewmilson andrewmilson force-pushed the 04-30-Implement_FieldOps_for_SIMD_backend branch from fd674a3 to fbb8a0b Compare May 15, 2024 13:11
@andrewmilson andrewmilson merged commit 8ed4d95 into dev May 15, 2024
22 checks passed
@andrewmilson andrewmilson deleted the 04-30-Implement_FieldOps_for_SIMD_backend branch May 15, 2024 13:15
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