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

Enabler struct #362

Merged
merged 1 commit into from
Jan 19, 2025
Merged

Enabler struct #362

merged 1 commit into from
Jan 19, 2025

Conversation

shaharsamocha7
Copy link
Contributor

@shaharsamocha7 shaharsamocha7 commented Jan 16, 2025

This change is Reviewable

Copy link
Contributor Author

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

Copy link

@alon-f alon-f 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 r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Gali-StarkWare and @shaharsamocha7)


stwo_cairo_prover/crates/prover/src/components/utils.rs line 88 at r1 (raw file):

/// The enabler column is a column of length `padding_offset.next_power_of_two()` where
/// 1. The first `padding_offset` elements set to 1;

elements are set

Code quote:

elements set

stwo_cairo_prover/crates/prover/src/components/utils.rs line 89 at r1 (raw file):

/// The enabler column is a column of length `padding_offset.next_power_of_two()` where
/// 1. The first `padding_offset` elements set to 1;
/// 2. Otherwise set to 0.

The rest are set to 0.

Code quote:

Otherwise set to 0.

Copy link
Contributor

@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: all files reviewed, 4 unresolved discussions (waiting on @shaharsamocha7)


stwo_cairo_prover/crates/prover/src/components/utils.rs line 87 at r1 (raw file):

}

/// The enabler column is a column of length `padding_offset.next_power_of_two()` where

Will it always be the case? It makes sense but I can imagine a scenario where we change the size of the table as part of an optimisation (uniting it with another bigger table for some reason).


stwo_cairo_prover/crates/prover/src/components/utils.rs line 101 at r1 (raw file):

    pub fn packed_at(&self, vec_row: usize) -> PackedM31 {
        let packed_row_offset = vec_row * N_LANES;
        PackedM31::from_array(array::from_fn(|i| {

We can use broadcast for cases of an all '0'/'1' arrays but I'm not sure how important is efficiency here. Non-blocking.

Copy link
Contributor Author

@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, 2 unresolved discussions (waiting on @alon-f and @Gali-StarkWare)


stwo_cairo_prover/crates/prover/src/components/utils.rs line 87 at r1 (raw file):

Previously, Gali-StarkWare wrote…

Will it always be the case? It makes sense but I can imagine a scenario where we change the size of the table as part of an optimisation (uniting it with another bigger table for some reason).

You mean padding by more than a power of two?
This is inherit in the protocol, so I don't think this should change.
But if it will, we will change the code when we need that :)


stwo_cairo_prover/crates/prover/src/components/utils.rs line 101 at r1 (raw file):

Previously, Gali-StarkWare wrote…

We can use broadcast for cases of an all '0'/'1' arrays but I'm not sure how important is efficiency here. Non-blocking.

Agree, but don't want to focus on optimizations ATM.

Copy link
Contributor

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

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @shaharsamocha7)

@shaharsamocha7 shaharsamocha7 merged commit 790307f into main Jan 19, 2025
7 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.

3 participants