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

Parallelize bit reverse m31. #855

Merged
merged 2 commits into from
Oct 1, 2024
Merged

Conversation

alonh5
Copy link
Contributor

@alonh5 alonh5 commented Sep 25, 2024

This change is Reviewable

Copy link
Contributor Author

alonh5 commented Sep 25, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @alonh5 and the rest of your teammates on Graphite Graphite

@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.06%. Comparing base (f1f1d18) to head (4018d1d).
Report is 7 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #855      +/-   ##
==========================================
- Coverage   92.21%   92.06%   -0.16%     
==========================================
  Files          90       90              
  Lines       12322    12659     +337     
  Branches    12322    12659     +337     
==========================================
+ Hits        11363    11654     +291     
- Misses        853      893      +40     
- Partials      106      112       +6     
Flag Coverage Δ
92.06% <100.00%> (-0.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Contributor

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

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


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

unsafe impl Send for UnsafePackedM31Array {}
unsafe impl Sync for UnsafePackedM31Array {}

Perhaps we can move to utils.rs and unify with the UnsafeMut types in the fft code

Suggestion:

pub struct UnsafeMut<T>(pub *mut T);
impl<T> UnsafeMut<T> {
    pub unsafe fn get(&self) -> *mut T {
        self.0
    }
}

unsafe impl<T> Send for UnsafeMut<T> {}
unsafe impl<T> Sync for UnsafeMut<T> {}

@alonh5 alonh5 force-pushed the 09-25-parallelize_bit_reverse_m31 branch from 544b225 to 0f629cb Compare September 25, 2024 10:28
Copy link
Contributor Author

@alonh5 alonh5 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 8 files reviewed, 1 unresolved discussion (waiting on @Alon-Ti, @andrewmilson, and @shaharsamocha7)


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

Previously, andrewmilson (Andrew Milson) wrote…

Perhaps we can move to utils.rs and unify with the UnsafeMut types in the fft code

Great idea. Done.

Copy link
Contributor

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

:lgtm: With comment

Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @Alon-Ti, @alonh5, and @shaharsamocha7)


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

unsafe impl<T> Send for UnsafeConst<T> {}
unsafe impl<T> Sync for UnsafeConst<T> {}

Maybe simd/utils.rs is better.
Not sure we want to expose these in our public API

Code quote:

// TODO(andrew): Examine usage of unsafe in SIMD FFT.
pub struct UnsafeMut<T: ?Sized>(pub *mut T);
impl<T: ?Sized> UnsafeMut<T> {
    /// # Safety
    ///
    /// Returns a raw mutable pointer.
    pub unsafe fn get(&self) -> *mut T {
        self.0
    }
}

unsafe impl<T: ?Sized> Send for UnsafeMut<T> {}
unsafe impl<T: ?Sized> Sync for UnsafeMut<T> {}

pub struct UnsafeConst<T>(pub *const T);
impl<T> UnsafeConst<T> {
    /// # Safety
    ///
    /// Returns a raw constant pointer.
    pub unsafe fn get(&self) -> *const T {
        self.0
    }
}

unsafe impl<T> Send for UnsafeConst<T> {}
unsafe impl<T> Sync for UnsafeConst<T> {}

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.

I moved UnsafeMut to simd utils

Reviewable status: 0 of 9 files reviewed, 2 unresolved discussions (waiting on @Alon-Ti, @alonh5, and @andrewmilson)

Copy link
Contributor

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

:lgtm:

Reviewable status: 0 of 9 files reviewed, 2 unresolved discussions (waiting on @Alon-Ti and @alonh5)

@shaharsamocha7 shaharsamocha7 merged commit 6250403 into dev Oct 1, 2024
15 of 16 checks passed
@shaharsamocha7 shaharsamocha7 deleted the 09-25-parallelize_bit_reverse_m31 branch October 1, 2024 07:25
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