From 4a3f9b9ec3179354e2b1313961c18c4ddfd5d8fb Mon Sep 17 00:00:00 2001 From: ashjeong Date: Wed, 16 Oct 2024 11:21:37 +0900 Subject: [PATCH 1/9] chore(math): fix comment typo --- tachyon/math/polynomials/univariate/radix2_evaluation_domain.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tachyon/math/polynomials/univariate/radix2_evaluation_domain.h b/tachyon/math/polynomials/univariate/radix2_evaluation_domain.h index 8954a7a34..85856c9b5 100644 --- a/tachyon/math/polynomials/univariate/radix2_evaluation_domain.h +++ b/tachyon/math/polynomials/univariate/radix2_evaluation_domain.h @@ -160,7 +160,7 @@ class Radix2EvaluationDomain // Reverse bits because |mat| is encoded in bit-reversed order size_t start = chunk_offset * chunk_size; F weight = this->size_inv_ * shift.Pow(start); - // NOTE: It is not possible to have empty chunk so this is safe + // NOTE: It is not possible to have an empty chunk so this is safe for (size_t row = start; row < start + len - 1; ++row) { mat.row(base::bits::ReverseBitsLen(row, this->log_size_of_group_)) *= weight; From 5bd0d1bb09f9bf30e6c530d1c76d9c2c12ce4160 Mon Sep 17 00:00:00 2001 From: ashjeong Date: Wed, 16 Oct 2024 11:22:15 +0900 Subject: [PATCH 2/9] refac(crypto): fix variable name typo from `ro_value` to `ro_values` --- tachyon/crypto/commitments/fri/verify.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tachyon/crypto/commitments/fri/verify.h b/tachyon/crypto/commitments/fri/verify.h index 5820ec153..f23bb3386 100644 --- a/tachyon/crypto/commitments/fri/verify.h +++ b/tachyon/crypto/commitments/fri/verify.h @@ -97,10 +97,11 @@ template ro_num_rows; - std::vector ro_value; + std::vector ro_values; size_t index = challenger.SampleBits(log_max_num_rows); VLOG(2) << "FRI(index[" << i << "]): " << index; - open_input(index, proof.query_proofs[i].input_proof, ro_num_rows, ro_value); + open_input(index, proof.query_proofs[i].input_proof, ro_num_rows, + ro_values); #if DCHECK_IS_ON() // Check reduced openings sorted by |num_rows| descending @@ -114,7 +115,7 @@ template Date: Wed, 16 Oct 2024 11:23:43 +0900 Subject: [PATCH 3/9] refac(crypto): ensure `shift` is constant --- tachyon/crypto/commitments/fri/two_adic_fri.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tachyon/crypto/commitments/fri/two_adic_fri.h b/tachyon/crypto/commitments/fri/two_adic_fri.h index 97ba1dc52..c29125377 100644 --- a/tachyon/crypto/commitments/fri/two_adic_fri.h +++ b/tachyon/crypto/commitments/fri/two_adic_fri.h @@ -385,7 +385,7 @@ class TwoAdicFRI { // https://hackmd.io/@vbuterin/barycentric_evaluation template static std::vector InterpolateCoset( - const Eigen::MatrixBase& coset_evals, F shift, + const Eigen::MatrixBase& coset_evals, const F shift, const ExtF& point) { TRACE_EVENT("Utils", "InterpolateCoset"); size_t num_rows = static_cast(coset_evals.rows()); From c1d08b345c09f3bddffbf018e1a9b6ada499fef0 Mon Sep 17 00:00:00 2001 From: ashjeong Date: Wed, 16 Oct 2024 11:27:48 +0900 Subject: [PATCH 4/9] fix(crypto): fix `ComputeInverseDenominators` faulty logic See [here](https://github.com/Plonky3/Plonky3/blob/97ab9d0a830a3a962aa1631d4035b3cf68c017a8/fri/src/two_adic_pcs.rs#L275). --- tachyon/crypto/commitments/fri/two_adic_fri.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tachyon/crypto/commitments/fri/two_adic_fri.h b/tachyon/crypto/commitments/fri/two_adic_fri.h index c29125377..83c462a09 100644 --- a/tachyon/crypto/commitments/fri/two_adic_fri.h +++ b/tachyon/crypto/commitments/fri/two_adic_fri.h @@ -337,17 +337,17 @@ class TwoAdicFRI { absl::Span>> matrices = matrices_by_round[round]; const OpeningPointsForRound& points = points_by_round[round]; - for (const Eigen::Map>& matrix : matrices) { + for (size_t i = 0; i < matrices.size(); ++i) { + const Eigen::Map>& matrix = matrices[i]; + const std::vector& point_list = points[i]; uint32_t log_num_rows = base::bits::CheckedLog2(static_cast(matrix.rows())); max_log_num_rows = std::max(max_log_num_rows, log_num_rows); - for (const std::vector& point_list : points) { - for (const ExtF& point : point_list) { - const auto [it, inserted] = - max_log_num_rows_for_point.try_emplace(point, log_num_rows); - if (!inserted) { - it->second = std::max(it->second, log_num_rows); - } + for (const ExtF& point : point_list) { + const auto [it, inserted] = + max_log_num_rows_for_point.try_emplace(point, log_num_rows); + if (!inserted) { + it->second = std::max(it->second, log_num_rows); } } } From caab54e3b3e286ff6b8858f3bc307f4069521644 Mon Sep 17 00:00:00 2001 From: ashjeong Date: Wed, 16 Oct 2024 22:08:24 +0900 Subject: [PATCH 5/9] refac(crypto): remove unused type --- .../field_merkle_tree/extension_field_merkle_tree_mmcs.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tachyon/crypto/commitments/merkle_tree/field_merkle_tree/extension_field_merkle_tree_mmcs.h b/tachyon/crypto/commitments/merkle_tree/field_merkle_tree/extension_field_merkle_tree_mmcs.h index 6ec45d34f..18f77b86b 100644 --- a/tachyon/crypto/commitments/merkle_tree/field_merkle_tree/extension_field_merkle_tree_mmcs.h +++ b/tachyon/crypto/commitments/merkle_tree/field_merkle_tree/extension_field_merkle_tree_mmcs.h @@ -22,8 +22,7 @@ class ExtensionFieldMerkleTreeMMCS final typename MixedMatrixCommitmentSchemeTraits::Commitment; using ProverData = typename MixedMatrixCommitmentSchemeTraits::ProverData; - using Digest = Commitment; - using Proof = std::vector; + using Proof = std::vector; ExtensionFieldMerkleTreeMMCS() = default; ExtensionFieldMerkleTreeMMCS(InnerMMCS&& inner) : inner_(std::move(inner)) {} From 779f070cccf7b67837ae74a5ae35e75634c08f60 Mon Sep 17 00:00:00 2001 From: ashjeong Date: Wed, 16 Oct 2024 22:21:45 +0900 Subject: [PATCH 6/9] chore(benchmark): fix invalid result message --- benchmark/fft/fft_benchmark.cc | 2 +- benchmark/fft/fft_benchmark_gpu.cc | 2 +- benchmark/fft_batch/fft_batch_benchmark.cc | 2 +- benchmark/fri/fri_benchmark.cc | 2 +- benchmark/msm/msm_benchmark.cc | 2 +- benchmark/msm/msm_benchmark_gpu.cc | 2 +- benchmark/poseidon/poseidon_benchmark.cc | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/benchmark/fft/fft_benchmark.cc b/benchmark/fft/fft_benchmark.cc index 1cbf8f7ad..01eab98ed 100644 --- a/benchmark/fft/fft_benchmark.cc +++ b/benchmark/fft/fft_benchmark.cc @@ -59,7 +59,7 @@ template void CheckResult(bool check_result, const PolyOrEvals& tachyon_result, const PolyOrEvals& vendor_result) { if (check_result) { - CHECK_EQ(tachyon_result, vendor_result) << "Results not matched"; + CHECK_EQ(tachyon_result, vendor_result) << "Results do not match"; } } diff --git a/benchmark/fft/fft_benchmark_gpu.cc b/benchmark/fft/fft_benchmark_gpu.cc index ea66e0924..094bd8594 100644 --- a/benchmark/fft/fft_benchmark_gpu.cc +++ b/benchmark/fft/fft_benchmark_gpu.cc @@ -79,7 +79,7 @@ void Run(const FFTConfig& config) { domain.get(), input, kShouldRecord, gpu_result); } if (config.check_results()) { - CHECK_EQ(cpu_result, gpu_result) << "Results not matched"; + CHECK_EQ(cpu_result, gpu_result) << "Results do not match"; } } diff --git a/benchmark/fft_batch/fft_batch_benchmark.cc b/benchmark/fft_batch/fft_batch_benchmark.cc index 1bdcc0a16..73606f9d7 100644 --- a/benchmark/fft_batch/fft_batch_benchmark.cc +++ b/benchmark/fft_batch/fft_batch_benchmark.cc @@ -26,7 +26,7 @@ void CheckResults(bool check_results, const math::RowMajorMatrix& tachyon_result, const math::RowMajorMatrix& vendor_result) { if (check_results) { - CHECK_EQ(tachyon_result, vendor_result) << "Results not matched"; + CHECK_EQ(tachyon_result, vendor_result) << "Results do not match"; } } diff --git a/benchmark/fri/fri_benchmark.cc b/benchmark/fri/fri_benchmark.cc index 563de3537..84e4cc164 100644 --- a/benchmark/fri/fri_benchmark.cc +++ b/benchmark/fri/fri_benchmark.cc @@ -39,7 +39,7 @@ template void CheckResult(bool check_results, const Result& tachyon_result, const Result& vendor_result) { if (check_results) { - CHECK_EQ(tachyon_result, vendor_result) << "Results not matched"; + CHECK_EQ(tachyon_result, vendor_result) << "Results do not match"; } } diff --git a/benchmark/msm/msm_benchmark.cc b/benchmark/msm/msm_benchmark.cc index 1144d46fd..b64855336 100644 --- a/benchmark/msm/msm_benchmark.cc +++ b/benchmark/msm/msm_benchmark.cc @@ -89,7 +89,7 @@ int RealMain(int argc, char** argv) { } if (config.check_results()) { - CHECK(results == results_vendor) << "Result not matched"; + CHECK(results == results_vendor) << "Results do not match"; } } diff --git a/benchmark/msm/msm_benchmark_gpu.cc b/benchmark/msm/msm_benchmark_gpu.cc index 2e4278d55..71d87863f 100644 --- a/benchmark/msm/msm_benchmark_gpu.cc +++ b/benchmark/msm/msm_benchmark_gpu.cc @@ -66,7 +66,7 @@ int RealMain(int argc, char** argv) { tachyon_bn254_g1_destroy_msm_gpu(msm_gpu); if (config.check_results()) { - CHECK(results_cpu == results_gpu) << "Result not matched"; + CHECK(results_cpu == results_gpu) << "Results do not match"; } reporter.Show(); diff --git a/benchmark/poseidon/poseidon_benchmark.cc b/benchmark/poseidon/poseidon_benchmark.cc index 76dab3108..b0966f7bb 100644 --- a/benchmark/poseidon/poseidon_benchmark.cc +++ b/benchmark/poseidon/poseidon_benchmark.cc @@ -47,7 +47,7 @@ int RealMain(int argc, char** argv) { runner.RunExternal(Vendor::Arkworks(), run_poseidon_arkworks); if (config.check_results()) { - CHECK_EQ(result, result_arkworks) << "Result not matched"; + CHECK_EQ(result, result_arkworks) << "Results do not match"; } reporter.AddAverageAsLastColumn(); From 095df2a68c8d7c2aa53c876cd6fa0bf49740b68b Mon Sep 17 00:00:00 2001 From: ashjeong Date: Thu, 17 Oct 2024 18:21:36 +0900 Subject: [PATCH 7/9] style(benchmark): fix lint errors Headers are moved out of the `TACHYON_CUDA` guard to ensure cpplint passes. --- benchmark/fft/fft_benchmark.cc | 2 ++ benchmark/fft/fft_benchmark_gpu.cc | 2 ++ benchmark/fft_batch/fft_batch_benchmark.cc | 2 ++ benchmark/msm/msm_benchmark.cc | 2 ++ benchmark/msm/msm_benchmark_gpu.cc | 3 ++- 5 files changed, 10 insertions(+), 1 deletion(-) diff --git a/benchmark/fft/fft_benchmark.cc b/benchmark/fft/fft_benchmark.cc index 01eab98ed..ccf96f1dd 100644 --- a/benchmark/fft/fft_benchmark.cc +++ b/benchmark/fft/fft_benchmark.cc @@ -2,6 +2,8 @@ #include #include +#include +#include // clang-format off #include "benchmark/fft/fft_config.h" diff --git a/benchmark/fft/fft_benchmark_gpu.cc b/benchmark/fft/fft_benchmark_gpu.cc index 094bd8594..ea523db1d 100644 --- a/benchmark/fft/fft_benchmark_gpu.cc +++ b/benchmark/fft/fft_benchmark_gpu.cc @@ -1,3 +1,5 @@ +#include +#include #if TACHYON_CUDA #include diff --git a/benchmark/fft_batch/fft_batch_benchmark.cc b/benchmark/fft_batch/fft_batch_benchmark.cc index 73606f9d7..b76375861 100644 --- a/benchmark/fft_batch/fft_batch_benchmark.cc +++ b/benchmark/fft_batch/fft_batch_benchmark.cc @@ -1,4 +1,6 @@ #include +#include +#include #include "absl/strings/substitute.h" diff --git a/benchmark/msm/msm_benchmark.cc b/benchmark/msm/msm_benchmark.cc index b64855336..62c4e0f40 100644 --- a/benchmark/msm/msm_benchmark.cc +++ b/benchmark/msm/msm_benchmark.cc @@ -1,4 +1,6 @@ #include +#include +#include // clang-format off #include "benchmark/msm/msm_config.h" diff --git a/benchmark/msm/msm_benchmark_gpu.cc b/benchmark/msm/msm_benchmark_gpu.cc index 71d87863f..328a025c1 100644 --- a/benchmark/msm/msm_benchmark_gpu.cc +++ b/benchmark/msm/msm_benchmark_gpu.cc @@ -1,5 +1,6 @@ -#if TACHYON_CUDA #include +#include +#if TACHYON_CUDA // clang-format off #include "benchmark/msm/msm_config.h" From edb728b2710776f366464608b4f4da0249b3259e Mon Sep 17 00:00:00 2001 From: ashjeong Date: Fri, 18 Oct 2024 11:39:12 +0900 Subject: [PATCH 8/9] refac(benchmark): remove unused header --- benchmark/fft/fft_benchmark.cc | 1 - benchmark/fft/fft_benchmark_gpu.cc | 1 - benchmark/fft_batch/fft_batch_benchmark.cc | 1 - 3 files changed, 3 deletions(-) diff --git a/benchmark/fft/fft_benchmark.cc b/benchmark/fft/fft_benchmark.cc index ccf96f1dd..090ad8f81 100644 --- a/benchmark/fft/fft_benchmark.cc +++ b/benchmark/fft/fft_benchmark.cc @@ -1,7 +1,6 @@ #include #include -#include #include #include diff --git a/benchmark/fft/fft_benchmark_gpu.cc b/benchmark/fft/fft_benchmark_gpu.cc index ea523db1d..52e148afe 100644 --- a/benchmark/fft/fft_benchmark_gpu.cc +++ b/benchmark/fft/fft_benchmark_gpu.cc @@ -1,7 +1,6 @@ #include #include #if TACHYON_CUDA -#include // clang-format off #include "benchmark/fft/fft_config.h" diff --git a/benchmark/fft_batch/fft_batch_benchmark.cc b/benchmark/fft_batch/fft_batch_benchmark.cc index b76375861..9a0733a7b 100644 --- a/benchmark/fft_batch/fft_batch_benchmark.cc +++ b/benchmark/fft_batch/fft_batch_benchmark.cc @@ -1,4 +1,3 @@ -#include #include #include From 1047906cfe9688e351f46bf9259fff032b759512 Mon Sep 17 00:00:00 2001 From: ashjeong Date: Wed, 16 Oct 2024 18:08:19 +0900 Subject: [PATCH 9/9] refac: rename variables for better clarity and understandability --- tachyon/crypto/commitments/fri/two_adic_fri.h | 7 ++++--- .../univariate/radix2_evaluation_domain.h | 6 +++--- .../polynomials/univariate/radix2_twiddle_cache.h | 12 ++++++------ 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/tachyon/crypto/commitments/fri/two_adic_fri.h b/tachyon/crypto/commitments/fri/two_adic_fri.h index 83c462a09..0bfbd6ab6 100644 --- a/tachyon/crypto/commitments/fri/two_adic_fri.h +++ b/tachyon/crypto/commitments/fri/two_adic_fri.h @@ -336,14 +336,15 @@ class TwoAdicFRI { for (size_t round = 0; round < num_rounds; ++round) { absl::Span>> matrices = matrices_by_round[round]; - const OpeningPointsForRound& points = points_by_round[round]; + const OpeningPointsForRound& points_vec_for_round = + points_by_round[round]; for (size_t i = 0; i < matrices.size(); ++i) { const Eigen::Map>& matrix = matrices[i]; - const std::vector& point_list = points[i]; + const std::vector& points_for_mat = points_vec_for_round[i]; uint32_t log_num_rows = base::bits::CheckedLog2(static_cast(matrix.rows())); max_log_num_rows = std::max(max_log_num_rows, log_num_rows); - for (const ExtF& point : point_list) { + for (const ExtF& point : points_for_mat) { const auto [it, inserted] = max_log_num_rows_for_point.try_emplace(point, log_num_rows); if (!inserted) { diff --git a/tachyon/math/polynomials/univariate/radix2_evaluation_domain.h b/tachyon/math/polynomials/univariate/radix2_evaluation_domain.h index 85856c9b5..aafcf3ec4 100644 --- a/tachyon/math/polynomials/univariate/radix2_evaluation_domain.h +++ b/tachyon/math/polynomials/univariate/radix2_evaluation_domain.h @@ -113,7 +113,7 @@ class Radix2EvaluationDomain // For the second half, we flip the DIT, working in bit-reversed order. ReverseMatrixIndexBits(mat); - RunParallelRowChunksReversed(mat, cache_->rev_roots_vec, + RunParallelRowChunksReversed(mat, cache_->bitrev_roots_vec, cache_->packed_roots_vec[1]); ReverseMatrixIndexBits(mat); } @@ -145,7 +145,7 @@ class Radix2EvaluationDomain // For the second half, we flip the DIT, working in bit-reversed order. ReverseMatrixIndexBits(mat); - RunParallelRowChunksReversed(mat, cache_->rev_inv_roots_vec, + RunParallelRowChunksReversed(mat, cache_->bitrev_inv_roots_vec, cache_->packed_inv_roots_vec[1]); // We skip the final bit-reversal, since the next FFT expects bit-reversed // input. @@ -189,7 +189,7 @@ class Radix2EvaluationDomain // For the second half, we flip the DIT, working in bit-reversed order. ReverseMatrixIndexBits(out); - domain->RunParallelRowChunksReversed(out, domain->cache_->rev_roots_vec, + domain->RunParallelRowChunksReversed(out, domain->cache_->bitrev_roots_vec, domain->cache_->packed_roots_vec[1]); if (reverse_at_last) { ReverseMatrixIndexBits(out); diff --git a/tachyon/math/polynomials/univariate/radix2_twiddle_cache.h b/tachyon/math/polynomials/univariate/radix2_twiddle_cache.h index 2dbf6bc3e..4a219b6b1 100644 --- a/tachyon/math/polynomials/univariate/radix2_twiddle_cache.h +++ b/tachyon/math/polynomials/univariate/radix2_twiddle_cache.h @@ -27,8 +27,8 @@ class Radix2TwiddleCache { struct Item { // For small prime fields - std::vector rev_roots_vec; - std::vector rev_inv_roots_vec; + std::vector bitrev_roots_vec; + std::vector bitrev_inv_roots_vec; std::vector> packed_roots_vec; std::vector> packed_inv_roots_vec; // For all finite fields @@ -80,16 +80,16 @@ class Radix2TwiddleCache { packed_inv_roots_vec[0].resize(vec_largest_size); packed_roots_vec[1].resize(vec_largest_size); packed_inv_roots_vec[1].resize(vec_largest_size); - rev_roots_vec = SwapBitRevElements(largest); - rev_inv_roots_vec = SwapBitRevElements(largest_inv); + bitrev_roots_vec = SwapBitRevElements(largest); + bitrev_inv_roots_vec = SwapBitRevElements(largest_inv); OMP_PARALLEL_FOR(size_t i = 0; i < vec_largest_size; ++i) { packed_roots_vec[0][i] = PackedPrimeField::Broadcast(largest[i]); packed_inv_roots_vec[0][i] = PackedPrimeField::Broadcast(largest_inv[i]); packed_roots_vec[1][i] = - PackedPrimeField::Broadcast(rev_roots_vec[i]); + PackedPrimeField::Broadcast(bitrev_roots_vec[i]); packed_inv_roots_vec[1][i] = - PackedPrimeField::Broadcast(rev_inv_roots_vec[i]); + PackedPrimeField::Broadcast(bitrev_inv_roots_vec[i]); } }