-
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 FieldOps for SIMD backend #593
Conversation
Codecov ReportAttention: Patch coverage is
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. |
51d9333
to
d2ca30c
Compare
0143448
to
9bd0fc4
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: 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,
}
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: 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.
d2ca30c
to
c3c6fd2
Compare
e9ed60d
to
aca433d
Compare
c3c6fd2
to
7ddb0a1
Compare
aca433d
to
6d71f67
Compare
7ddb0a1
to
2c82ea1
Compare
6d71f67
to
455686c
Compare
2c82ea1
to
d8abf68
Compare
455686c
to
ea02309
Compare
d8abf68
to
063029c
Compare
ea02309
to
567e20c
Compare
063029c
to
2ca8966
Compare
567e20c
to
b6daa79
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.
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)
b8a5161
to
92fb94e
Compare
0f31f09
to
9509a53
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 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!()
}
}
9509a53
to
8db394e
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: 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.
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 4 of 4 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @andrewmilson)
92fb94e
to
a82d991
Compare
8db394e
to
0990999
Compare
a82d991
to
d555471
Compare
0990999
to
d79b8b3
Compare
d555471
to
7c485c7
Compare
d79b8b3
to
aa87739
Compare
7c485c7
to
d7299c5
Compare
aa87739
to
fd674a3
Compare
fd674a3
to
fbb8a0b
Compare
This change is