Skip to content

Commit

Permalink
Reassigned TODO(spapini)s.
Browse files Browse the repository at this point in the history
  • Loading branch information
Alon-Ti committed Sep 16, 2024
1 parent f787197 commit 7dc7f0d
Show file tree
Hide file tree
Showing 23 changed files with 32 additions and 41 deletions.
4 changes: 2 additions & 2 deletions crates/prover/src/constraint_framework/component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,8 @@ impl<E: FrameworkEval> ComponentProver<SimdBackend> for FrameworkComponent<E> {
let component_evals = trace.evals.sub_tree(&self.trace_locations);

// Extend trace if necessary.
// TODO(spapini): Don't extend when eval_size < committed_size. Instead, pick a good
// subdomain.
// TODO: Don't extend when eval_size < committed_size. Instead, pick a good
// subdomain. (For larger blowup factors).
let need_to_extend = component_evals
.iter()
.flatten()
Expand Down
2 changes: 1 addition & 1 deletion crates/prover/src/constraint_framework/logup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ impl<const N: usize> LookupElements<N> {
})
- EF::from(self.z)
}
// TODO(spapini): Try to remove this.

pub fn dummy() -> Self {
Self {
z: SecureField::one(),
Expand Down
2 changes: 1 addition & 1 deletion crates/prover/src/constraint_framework/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use crate::core::fields::FieldExpOps;

/// A trait for evaluating expressions at some point or row.
pub trait EvalAtRow {
// TODO(spapini): Use a better trait for these, like 'Algebra' or something.
// TODO(Ohad): Use a better trait for these, like 'Algebra' or something.
/// The field type holding values of columns for the component. These are the inputs to the
/// constraints. It might be [BaseField] packed types, or even [SecureField], when evaluating
/// the columns out of domain.
Expand Down
2 changes: 1 addition & 1 deletion crates/prover/src/constraint_framework/simd_domain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ impl<'a> EvalAtRow for SimdDomainEvaluator<'a> {
type F = VeryPackedBaseField;
type EF = VeryPackedSecureField;

// TODO(spapini): Remove all boundary checks.
// TODO(Ohad): Add debug boundary checks.
fn next_interaction_mask<const N: usize>(
&mut self,
interaction: usize,
Expand Down
1 change: 0 additions & 1 deletion crates/prover/src/core/air/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ pub mod mask;
/// evaluate the constraints.
/// For instance, all interaction elements are assumed to be present in it.
/// Therefore, an AIR is generated only after the initial trace commitment phase.
// TODO(spapini): consider renaming this struct.
pub trait Air {
fn components(&self) -> Vec<&dyn Component>;
}
Expand Down
1 change: 0 additions & 1 deletion crates/prover/src/core/backend/cpu/fri.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ use crate::core::poly::line::LineEvaluation;
use crate::core::poly::twiddles::TwiddleTree;
use crate::core::poly::BitReversedOrder;

// TODO(spapini): Optimized these functions as well.
impl FriOps for CpuBackend {
fn fold_line(
eval: &LineEvaluation<Self>,
Expand Down
1 change: 0 additions & 1 deletion crates/prover/src/core/backend/cpu/grind.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ use crate::core::proof_of_work::GrindOps;

impl<C: Channel> GrindOps<C> for CpuBackend {
fn grind(channel: &C, pow_bits: u32) -> u64 {
// TODO(spapini): This is a naive implementation. Optimize it.
let mut nonce = 0;
loop {
let mut channel = channel.clone();
Expand Down
2 changes: 1 addition & 1 deletion crates/prover/src/core/backend/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ pub trait ColumnOps<T> {

pub type Col<B, T> = <B as ColumnOps<T>>::Column;

// TODO(spapini): Consider removing the generic parameter and only support BaseField.
// TODO(alont): Consider removing the generic parameter and only support BaseField.
pub trait Column<T>: Clone + Debug + FromIterator<T> {
/// Creates a new column of zeros with the given length.
fn zeros(len: usize) -> Self;
Expand Down
4 changes: 2 additions & 2 deletions crates/prover/src/core/backend/simd/bit_reverse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ pub fn bit_reverse_m31(data: &mut [PackedBaseField]) {
let log_size = data.len().ilog2();
let a_bits = log_size - 2 * W_BITS - VEC_BITS;

// TODO(spapini): when doing multithreading, do it over a.
// TODO(AlonH): when doing multithreading, do it over a.
for a in 0u32..1 << a_bits {
for w_l in 0u32..1 << W_BITS {
let w_l_rev = w_l.reverse_bits() >> (u32::BITS - W_BITS);
Expand All @@ -66,7 +66,7 @@ pub fn bit_reverse_m31(data: &mut [PackedBaseField]) {
}

// Read first chunk.
// TODO(spapini): Think about optimizing a_bits.
// TODO(andrew): Think about optimizing a_bits. What does this mean?
let chunk0 = array::from_fn(|i| unsafe {
*data.get_unchecked(idx + (i << (2 * W_BITS + a_bits)))
});
Expand Down
13 changes: 6 additions & 7 deletions crates/prover/src/core/backend/simd/circle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ impl SimdBackend {
}
}

// TODO(spapini): Everything is returned in redundant representation, where values can also be P.
// TODO(shahars): Everything is returned in redundant representation, where values can also be P.
// Decide if and when it's ok and what to do if it's not.
impl PolyOps for SimdBackend {
// The twiddles type is i32, and not BaseField. This is because the fast AVX mul implementation
Expand All @@ -131,7 +131,7 @@ impl PolyOps for SimdBackend {
coset: CanonicCoset,
values: Col<Self, BaseField>,
) -> CircleEvaluation<Self, BaseField, BitReversedOrder> {
// TODO(spapini): Optimize.
// TODO(Ohad): Optimize.
let eval = CpuBackend::new_canonical_ordered(coset, values.into_cpu_vec());
CircleEvaluation::new(
eval.domain,
Expand All @@ -157,7 +157,7 @@ impl PolyOps for SimdBackend {
);
}

// TODO(spapini): Fuse this multiplication / rotation.
// TODO(alont): Cache this inversion.
let inv = PackedBaseField::broadcast(BaseField::from(eval.domain.size()).inverse());
values.data.iter_mut().for_each(|x| *x *= inv);

Expand Down Expand Up @@ -211,7 +211,7 @@ impl PolyOps for SimdBackend {
}

fn extend(poly: &CirclePoly<Self>, log_size: u32) -> CirclePoly<Self> {
// TODO(spapini): Optimize or get rid of extend.
// TODO(shahars): Get rid of extends.
poly.evaluate(CanonicCoset::new(log_size).circle_domain())
.interpolate()
}
Expand All @@ -221,8 +221,7 @@ impl PolyOps for SimdBackend {
domain: CircleDomain,
twiddles: &TwiddleTree<Self>,
) -> CircleEvaluation<Self, BaseField, BitReversedOrder> {
// TODO(spapini): Precompute twiddles.
// TODO(spapini): Handle small cases.
// TODO(AlonH): Handle small cases.
let log_size = domain.log_size();
let fft_log_size = poly.log_size();
assert!(
Expand Down Expand Up @@ -279,7 +278,7 @@ impl PolyOps for SimdBackend {
let mut twiddles = Vec::with_capacity(coset.size());
let mut itwiddles = Vec::with_capacity(coset.size());

// TODO(spapini): Optimize.
// TODO(alont): Optimize.
for layer in &rfft::get_twiddle_dbls(coset) {
twiddles.extend(layer);
}
Expand Down
2 changes: 1 addition & 1 deletion crates/prover/src/core/backend/simd/fft/ifft.rs
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ pub fn vecwise_ibutterflies(
twiddle2_dbl: [u32; 4],
twiddle3_dbl: [u32; 2],
) -> (PackedBaseField, PackedBaseField) {
// TODO(spapini): The permute can be fused with the _mm512_srli_epi64 inside the butterfly.
// TODO(andrew): Can the permute be fused with the _mm512_srli_epi64 inside the butterfly?

// Each `ibutterfly` take 2 512-bit registers, and does 16 butterflies element by element.
// We need to permute the 512-bit registers to get the right order for the butterflies.
Expand Down
2 changes: 1 addition & 1 deletion crates/prover/src/core/backend/simd/fft/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ pub const CACHED_FFT_LOG_SIZE: u32 = 16;

pub const MIN_FFT_LOG_SIZE: u32 = 5;

// TODO(spapini): FFTs return a redundant representation, that can get the value P. need to reduce
// TODO(andrew): FFTs return a redundant representation, that can get the value P. need to reduce
// it somewhere.

/// Transposes the SIMD vectors in the given array.
Expand Down
3 changes: 1 addition & 2 deletions crates/prover/src/core/backend/simd/fft/rfft.rs
Original file line number Diff line number Diff line change
Expand Up @@ -365,8 +365,7 @@ pub fn vecwise_butterflies(
twiddle2_dbl: [u32; 4],
twiddle3_dbl: [u32; 2],
) -> (PackedBaseField, PackedBaseField) {
// TODO(spapini): Compute twiddle0 from twiddle1.
// TODO(spapini): The permute can be fused with the _mm512_srli_epi64 inside the butterfly.
// TODO(andrew): Can the permute be fused with the _mm512_srli_epi64 inside the butterfly?
// The implementation is the exact reverse of vecwise_ibutterflies().
// See the comments in its body for more info.
let t = simd_swizzle!(
Expand Down
1 change: 1 addition & 0 deletions crates/prover/src/core/backend/simd/fri.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use crate::core::poly::twiddles::TwiddleTree;
use crate::core::poly::utils::domain_line_twiddles_from_tree;
use crate::core::poly::BitReversedOrder;

// TODO(andrew) Is this optimized?
impl FriOps for SimdBackend {
fn fold_line(
eval: &LineEvaluation<Self>,
Expand Down
4 changes: 2 additions & 2 deletions crates/prover/src/core/backend/simd/grind.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const GRIND_HI_BITS: u32 = 64 - GRIND_LOW_BITS;

impl GrindOps<Blake2sChannel> for SimdBackend {
fn grind(channel: &Blake2sChannel, pow_bits: u32) -> u64 {
// TODO(spapini): support more than 32 bits.
// TODO(first): support more than 32 bits.
assert!(pow_bits <= 32, "pow_bits > 32 is not supported");
let digest = channel.digest();
let digest: &[u32] = cast_slice(&digest.0[..]);
Expand Down Expand Up @@ -62,7 +62,7 @@ fn grind_blake(digest: &[u32], hi: u64, pow_bits: u32) -> Option<u64> {
None
}

// TODO(spapini): This is a naive implementation. Optimize it.
// TODO(shahars): This is a naive implementation. Optimize it.
#[cfg(not(target_arch = "wasm32"))]
impl GrindOps<Poseidon252Channel> for SimdBackend {
fn grind(channel: &Poseidon252Channel, pow_bits: u32) -> u64 {
Expand Down
7 changes: 4 additions & 3 deletions crates/prover/src/core/backend/simd/quotients.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ impl QuotientOps for SimdBackend {
log_blowup_factor: u32,
) -> SecureEvaluation<Self, BitReversedOrder> {
// Split the domain into a subdomain and a shift coset.
// TODO(spapini): Move to the caller when Columns support slices.
// TODO(andrew): Move to the caller when Columns support slices.
let (subdomain, mut subdomain_shifts) = domain.split(log_blowup_factor);
if subdomain.log_size() < LOG_N_LANES + 2 {
// Fall back to the CPU backend for small domains.
Expand Down Expand Up @@ -83,7 +83,7 @@ impl QuotientOps for SimdBackend {
);

// Extend the evaluation to the full domain.
// TODO(spapini): Try to optimize out all these copies.
// TODO(Ohad): Try to optimize out all these copies.
for (ci, &c) in subdomain_shifts.iter().enumerate() {
let subdomain = subdomain.shift(c);

Expand Down Expand Up @@ -123,7 +123,8 @@ fn accumulate_quotients_on_subdomain(
.array_chunks::<4>()
.enumerate()
{
// TODO(spapini): Use optimized domain iteration.
// TODO(andrew): Spapini said: Use optimized domain iteration. Is there a better way to do
// this?
let (y01, _) = points[0].y.deinterleave(points[1].y);
let (y23, _) = points[2].y.deinterleave(points[3].y);
let (spaced_ys, _) = y01.deinterleave(y23);
Expand Down
3 changes: 1 addition & 2 deletions crates/prover/src/core/channel/blake2s.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ impl Channel for Blake2sChannel {
msg[1] = (nonce >> 32) as u32;
let res = compress(std::array::from_fn(|i| digest[i]), msg, 0, 0, 0, 0);

// TODO(shahars) Channel should always finalize hash.
self.update_digest(unsafe { std::mem::transmute(res) });
}

Expand Down Expand Up @@ -105,8 +106,6 @@ impl Channel for Blake2sChannel {

hash_input.extend_from_slice(&padded_counter);

// TODO(spapini): Are we worried about this drawing hash colliding with mix_digest?

self.channel_time.inc_sent();
Blake2sHasher::hash(&hash_input).into()
}
Expand Down
5 changes: 2 additions & 3 deletions crates/prover/src/core/channel/poseidon252.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ impl Poseidon252Channel {
res
}

// TODO(spapini): Understand if we really need uniformity here.
// TODO(shahars): Understand if we really need uniformity here.
/// Generates a close-to uniform random vector of BaseField elements.
fn draw_base_felts(&mut self) -> [BaseField; 8] {
let shift = (1u64 << 31).into();
Expand Down Expand Up @@ -61,7 +61,6 @@ impl Channel for Poseidon252Channel {
u128::from_le_bytes(std::array::from_fn(|i| bytes[i])).trailing_zeros()
}

// TODO(spapini): Optimize.
fn mix_felts(&mut self, felts: &[SecureField]) {
let shift = (1u64 << 31).into();
let mut res = Vec::with_capacity(felts.len() / 2 + 2);
Expand All @@ -77,7 +76,7 @@ impl Channel for Poseidon252Channel {
);
}

// TODO(spapini): do we need length padding?
// TODO(shahars): do we need length padding?
self.update_digest(poseidon_hash_many(&res));
}

Expand Down
2 changes: 1 addition & 1 deletion crates/prover/src/core/fields/secure_column.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ pub struct SecureColumnByCoords<B: FieldOps<BaseField>> {
pub columns: [Col<B, BaseField>; SECURE_EXTENSION_DEGREE],
}
impl SecureColumnByCoords<CpuBackend> {
// TODO(spapini): Remove when we no longer use CircleEvaluation<SecureField>.
// TODO(first): Remove.
pub fn to_vec(&self) -> Vec<SecureField> {
(0..self.len()).map(|i| self.at(i)).collect()
}
Expand Down
5 changes: 1 addition & 4 deletions crates/prover/src/core/fri.rs
Original file line number Diff line number Diff line change
Expand Up @@ -686,7 +686,6 @@ impl<H: MerkleHasher> FriLayerVerifier<H> {
commitment,
vec![self.domain.log_size(); SECURE_EXTENSION_DEGREE],
);
// TODO(spapini): Propagate error.
merkle_verifier
.verify(
[(self.domain.log_size(), decommitment_positions)]
Expand Down Expand Up @@ -784,8 +783,6 @@ struct FriLayerProver<B: FriOps + MerkleOps<H>, H: MerkleHasher> {

impl<B: FriOps + MerkleOps<H>, H: MerkleHasher> FriLayerProver<B, H> {
fn new(evaluation: LineEvaluation<B>) -> Self {
// TODO(spapini): Commit on slice.
// TODO(spapini): Merkle tree in backend.
let merkle_tree = MerkleProver::commit(evaluation.values.columns.iter().collect_vec());
#[allow(unreachable_code)]
FriLayerProver {
Expand Down Expand Up @@ -822,7 +819,7 @@ impl<B: FriOps + MerkleOps<H>, H: MerkleHasher> FriLayerProver<B, H> {
}

let commitment = self.merkle_tree.root();
// TODO(spapini): Use _evals.
// TODO(andrew): Use _evals.
let (_evals, decommitment) = self.merkle_tree.decommit(
[(self.evaluation.len().ilog2(), decommit_positions)]
.into_iter()
Expand Down
2 changes: 0 additions & 2 deletions crates/prover/src/core/pcs/verifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,6 @@ impl<MC: MerkleChannel> CommitmentSchemeVerifier<MC> {
})
.flatten();

// TODO(spapini): Properly defined column log size and dinstinguish between poly and
// commitment.
let fri_answers = fri_answers(
self.column_log_sizes().flatten().into_iter().collect(),
&samples,
Expand Down
3 changes: 2 additions & 1 deletion crates/prover/src/core/poly/circle/evaluation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,9 @@ impl<B: FieldOps<F>, F: ExtensionOf<BaseField>, EvalOrder> CircleEvaluation<B, F

// Note: The concrete implementation of the poly operations is in the specific backend used.
// For example, the CPU backend implementation is in `src/core/backend/cpu/poly.rs`.
// TODO(andrew) Can we remove NaturalOrder? Is it used in FRI?
impl<F: ExtensionOf<BaseField>, B: FieldOps<F>> CircleEvaluation<B, F, NaturalOrder> {
// TODO(spapini): Remove. Is this even used.
// TODO(alont): Remove. Is this even used.
pub fn get_at(&self, point_index: CirclePointIndex) -> F {
self.values
.at(self.domain.find(point_index).expect("Not in domain"))
Expand Down
2 changes: 1 addition & 1 deletion crates/prover/src/core/poly/circle/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::core::poly::BitReversedOrder;

/// Operations on BaseField polynomials.
pub trait PolyOps: FieldOps<BaseField> + Sized {
// TODO(spapini): Use a column instead of this type.
// TODO(alont): Use a column instead of this type.
/// The type for precomputed twiddles.
type Twiddles;

Expand Down

0 comments on commit 7dc7f0d

Please sign in to comment.