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

avx blake #518

Merged
merged 1 commit into from
Mar 25, 2024
Merged

avx blake #518

merged 1 commit into from
Mar 25, 2024

Conversation

spapinistarkware
Copy link
Contributor

@spapinistarkware spapinistarkware commented Mar 21, 2024

This change is Reviewable

Copy link
Contributor Author

spapinistarkware commented Mar 21, 2024

@codecov-commenter
Copy link

codecov-commenter commented Mar 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.40%. Comparing base (83002ed) to head (da3de7d).

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #518      +/-   ##
==========================================
+ Coverage   95.17%   95.40%   +0.22%     
==========================================
  Files          63       65       +2     
  Lines        9229     9678     +449     
  Branches     9229     9678     +449     
==========================================
+ Hits         8784     9233     +449     
  Misses        387      387              
  Partials       58       58              

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

Copy link
Collaborator

@ohad-starkware ohad-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 6 files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)


src/commitment_scheme/blake2_hash.rs line 108 at r1 (raw file):

    fn finalize(mut self) -> Blake2sHash {
        if self.current_len != 0 {
            self.update(&[0; 64]);

finalize is not a regular compression,
maybe we should expose a finalize without a finalization flag


src/commitment_scheme/blake2_hash.rs line 114 at r1 (raw file):

    fn finalize_reset(&mut self) -> Blake2sHash {
        let hash = self.clone().finalize();

Suggestion:

let hash = Blake2sHash(unsafe{ std::mem::transmute_copy(&self.state)})

src/commitment_scheme/blake2s_avx.rs line 216 at r1 (raw file):

}

/// # Safety

doc/make private


src/commitment_scheme/blake2s_ref.rs line 1 at r1 (raw file):

//! Based on bla bla's impl. TODO: add the link.

just a reminder to not forget these


src/core/proof_of_work.rs line 100 at r1 (raw file):

    #[test]
    fn test_verify_proof_of_work_success() {

why delete this?

Copy link
Contributor Author

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

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @ohad-starkware)


src/commitment_scheme/blake2_hash.rs line 108 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

finalize is not a regular compression,
maybe we should expose a finalize without a finalization flag

For stark, we dont care about finalization.

Copy link
Contributor Author

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

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @ohad-starkware)


src/core/proof_of_work.rs line 100 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

why delete this?

This test depends on concrete values of the hash, so it's not really a POW test. The 3rd tests in this file is enough IMO

@spapinistarkware spapinistarkware force-pushed the spapini/03-20-avx_blake branch 7 times, most recently from e747345 to ad09eca Compare March 21, 2024 12:22
Copy link
Collaborator

@ohad-starkware ohad-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 2 of 5 files at r2.
Reviewable status: 2 of 7 files reviewed, 3 unresolved discussions (waiting on @spapinistarkware)


src/commitment_scheme/blake2_hash.rs line 94 at r2 (raw file):

    fn update(&mut self, mut data: &[u8]) {
        while self.pending_len + data.len() >= 64 {

use Self::BLOCK_SIZE
(in the lower level stuff I think raw numbers is fine)


src/commitment_scheme/blake2_hash.rs line 101 at r2 (raw file):

            let words =
                unsafe { std::mem::transmute::<&[u8; 64], &[u32; 16]>(&self.pending_buffer) };
            blake2s_ref::compress(&mut self.state, words, 0, 0, 0, 0);

if count arguments = 0 is intentional, document


src/commitment_scheme/blake2_hash.rs line 139 at r2 (raw file):

            dst = dst.add(16 * 32);
        }
        // Handle the remainder.

below too

Suggestion:

        }
        
        // Handle the remainder.

src/commitment_scheme/blake2_hash.rs line 163 at r2 (raw file):

unsafe fn compress16(inputs: &[*const u8; 16], single_input_length_bytes: usize) -> [u8; 16 * 32] {
    let mut states = [set1(0); 8];
    let mut inputs = inputs.map(|input| input);

or take ownership

Suggestion:

*inputs

src/commitment_scheme/blake2_hash.rs line 166 at r2 (raw file):

    // Compress in chunks of 64 bytes.
    for _j in (0..single_input_length_bytes).step_by(64) {

Suggestion:

_

Copy link
Contributor Author

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

Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on @ohad-starkware)


src/commitment_scheme/blake2_hash.rs line 139 at r2 (raw file):

Previously, ohad-starkware (Ohad) wrote…

below too

Done.


src/commitment_scheme/blake2_hash.rs line 163 at r2 (raw file):

Previously, ohad-starkware (Ohad) wrote…

or take ownership

Done.


src/commitment_scheme/blake2_hash.rs line 166 at r2 (raw file):

    // Compress in chunks of 64 bytes.
    for _j in (0..single_input_length_bytes).step_by(64) {

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.

Reviewable status: 0 of 7 files reviewed, 8 unresolved discussions (waiting on @andrewmilson, @ohad-starkware, and @spapinistarkware)


src/commitment_scheme/blake2_hash.rs line 67 at r4 (raw file):

/// Wrapper for the blake2s Hashing functionalities.
/// This blake2s hasher is intended to be sued only when the structure of the input is agreed upon.

used

Code quote:

sued

src/commitment_scheme/blake2_hash.rs line 71 at r4 (raw file):

/// The blake2s permutation is used as a hash chain, with no counts, flags or proper padding.
#[derive(Clone, Debug)]
pub struct Blake2sHasher {

We should do finialize for the channel hash.
Only the merkle can reduce it

Code quote:

pub struct Blake2sHasher {

src/commitment_scheme/blake2_hash.rs line 73 at r4 (raw file):

pub struct Blake2sHasher {
    state: [u32; 8],
    pending_buffer: [u8; 64],

Document that this is double than the state size.

Code quote:

pending_buffer: [u8; 64],

src/commitment_scheme/hasher.rs line 81 at r4 (raw file):

            })
    }
}

revert

Code quote:

    ) {
        data.iter()
            .map(|p| std::slice::from_raw_parts(*p, single_input_length_bytes))
            .zip(
                dst.iter()
                    .map(|p| std::slice::from_raw_parts_mut(*p, Self::OUTPUT_SIZE)),
            )
            .for_each(|(input, out)| {
                let mut hasher = Self::new();
                hasher.update(input);
                out.clone_from_slice(&hasher.finalize().into());
            })
    }
}

src/core/proof_of_work.rs line 106 at r4 (raw file):

        proof_of_work_prover.verify(&mut channel, &proof).unwrap();
    }

revert

Code quote:

    #[test]
    fn test_verify_proof_of_work_success() {
        let mut channel = Blake2sChannel::new(Blake2sHash::from(vec![0; 32]));
        let proof_of_work_prover = ProofOfWork { n_bits: 11 };
        let proof = ProofOfWorkProof { nonce: 133 };

        proof_of_work_prover.verify(&mut channel, &proof).unwrap();
    }

@spapinistarkware spapinistarkware force-pushed the spapini/03-20-avx_blake branch 2 times, most recently from 9f67f18 to 884140b Compare March 25, 2024 05:19
Copy link
Contributor Author

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

Reviewable status: 0 of 7 files reviewed, 8 unresolved discussions (waiting on @andrewmilson, @ohad-starkware, and @shaharsamocha7)


src/commitment_scheme/blake2_hash.rs line 67 at r4 (raw file):

Previously, shaharsamocha7 wrote…

used

Done.


src/commitment_scheme/blake2_hash.rs line 71 at r4 (raw file):

Previously, shaharsamocha7 wrote…

We should do finialize for the channel hash.
Only the merkle can reduce it

Done.


src/commitment_scheme/blake2_hash.rs line 73 at r4 (raw file):

Previously, shaharsamocha7 wrote…

Document that this is double than the state size.

Done.


src/commitment_scheme/hasher.rs line 81 at r4 (raw file):

Previously, shaharsamocha7 wrote…

revert

Done.


src/core/proof_of_work.rs line 106 at r4 (raw file):

Previously, shaharsamocha7 wrote…

revert

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.

Reviewed 1 of 5 files at r2, 3 of 3 files at r5.
Reviewable status: 4 of 7 files reviewed, 8 unresolved discussions (waiting on @andrewmilson and @ohad-starkware)

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 1 of 3 files at r4, all commit messages.
Reviewable status: 5 of 7 files reviewed, 6 unresolved discussions (waiting on @andrewmilson, @ohad-starkware, and @spapinistarkware)

a discussion (no related file):
move this logic to avx_backend



src/commitment_scheme/blake2s_avx.rs line 179 at r5 (raw file):

}

const EVENS_CONCAT_EVENS: __m512i = unsafe {

Can this be unified with the fft into transpose_utils?

Code quote:

const EVENS_CONCAT_EVENS: __m512i = unsafe {

src/commitment_scheme/blake2s_avx.rs line 209 at r5 (raw file):

/// # Safety
pub unsafe fn transpose_msgs(mut data: [__m512i; 16]) -> [__m512i; 16] {
    // Transpose by applying 4 times the index permutation:

applying 4 times rotr(index, 1)
Maybe write the result of applying that 4 times

Code quote:

// Transpose by applying 4 times the index permutation:

@spapinistarkware spapinistarkware force-pushed the spapini/03-20-avx_blake branch from 884140b to f1c6f1c Compare March 25, 2024 09:21
Copy link
Contributor Author

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

Reviewable status: 5 of 7 files reviewed, 6 unresolved discussions (waiting on @andrewmilson, @ohad-starkware, and @shaharsamocha7)

a discussion (no related file):

Previously, shaharsamocha7 wrote…

move this logic to avx_backend

Done.



src/commitment_scheme/blake2s_avx.rs line 179 at r5 (raw file):

Previously, shaharsamocha7 wrote…

Can this be unified with the fft into transpose_utils?

Done.


src/commitment_scheme/blake2s_avx.rs line 209 at r5 (raw file):

Previously, shaharsamocha7 wrote…

applying 4 times rotr(index, 1)
Maybe write the result of applying that 4 times

Done.

@spapinistarkware spapinistarkware force-pushed the spapini/03-20-avx_blake branch from f1c6f1c to ec5d9d8 Compare March 25, 2024 09:54
@shaharsamocha7
Copy link
Collaborator

src/core/backend/avx512/blake2s_avx.rs line 190 at r6 (raw file):

    //   abcd:0123 => 3abc:d012
    // where abcd:0123 denotes that 0b 0123 word in the 0b abcd chunk.
    // In other words, rotate the index to the right by 1.

Each _m512i chunk contains 16 u32 words.
Index abcd:xyzw, refers to a specific word in data as follows:
abcd - chunk index (in base 2)
xyzw - word offset (in base 2)

Code quote:

    // Transpose by applying 4 times the index permutation:
    //   abcd:0123 => 3abc:d012
    // where abcd:0123 denotes that 0b 0123 word in the 0b abcd chunk.
    // In other words, rotate the index to the right by 1.

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 1 of 3 files at r4, 4 of 5 files at r6, all commit messages.
Reviewable status: 9 of 10 files reviewed, 4 unresolved discussions (waiting on @andrewmilson, @ohad-starkware, and @spapinistarkware)

@spapinistarkware spapinistarkware force-pushed the spapini/03-20-avx_blake branch from ec5d9d8 to da3de7d Compare March 25, 2024 11:46
Copy link
Contributor Author

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

Reviewable status: 9 of 10 files reviewed, 4 unresolved discussions (waiting on @andrewmilson, @ohad-starkware, and @shaharsamocha7)


src/core/backend/avx512/blake2s_avx.rs line 190 at r6 (raw file):

Previously, shaharsamocha7 wrote…

Each _m512i chunk contains 16 u32 words.
Index abcd:xyzw, refers to a specific word in data as follows:
abcd - chunk index (in base 2)
xyzw - word offset (in base 2)

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 1 of 5 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @andrewmilson and @ohad-starkware)

@spapinistarkware spapinistarkware merged commit 9dbe5bc into dev Mar 25, 2024
9 of 10 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