Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces new benchmarks for the FRI (Fast Reed-Solomon Interactive Oracle Proofs of Proximity) fold operation, covering both CPU and CUDA implementations. These benchmarks aim to measure the performance of Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds new benchmarks for FRI folding, including a CUDA version, which is a valuable addition for performance tracking. The changes are generally well-implemented. I've identified a couple of areas for improvement: the CMake configuration for running benchmarks should be updated to include the new CUDA benchmark, and there's a minor inconsistency in random number generation in one of the new benchmark files that should be addressed for better reproducibility. The other changes for CUDA compatibility and error handling are correct and improve the codebase.
| add_custom_target(run_benchmarks | ||
| COMMAND bench_field | ||
| COMMAND transpose_benchmark | ||
| COMMAND columnwise_dot_product | ||
| DEPENDS bench_field transpose_benchmark columnwise_dot_product | ||
| COMMAND bench_fold_even_odd | ||
| DEPENDS bench_field transpose_benchmark columnwise_dot_product bench_fold_even_odd | ||
| WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} | ||
| COMMENT "Running benchmarks..." | ||
| ) |
There was a problem hiding this comment.
The run_benchmarks custom target does not include the bench_fold_even_odd_cuda executable when ENABLE_CUDA is enabled. This means the CUDA benchmark will be built but not run with the run_benchmarks target. To ensure all built benchmarks are run, you should conditionally add the CUDA benchmark to the target.
if(ENABLE_CUDA)
add_custom_target(run_benchmarks
COMMAND bench_field
COMMAND transpose_benchmark
COMMAND columnwise_dot_product
COMMAND bench_fold_even_odd
COMMAND bench_fold_even_odd_cuda
DEPENDS bench_field transpose_benchmark columnwise_dot_product bench_fold_even_odd bench_fold_even_odd_cuda
WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
COMMENT "Running benchmarks..."
)
else()
add_custom_target(run_benchmarks
COMMAND bench_field
COMMAND transpose_benchmark
COMMAND columnwise_dot_product
COMMAND bench_fold_even_odd
DEPENDS bench_field transpose_benchmark columnwise_dot_product bench_fold_even_odd
WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
COMMENT "Running benchmarks..."
)
endif()
src/benches/bench_fold_even_odd.cpp
Outdated
| std::mt19937_64 local_rng(n); | ||
| std::uniform_int_distribution<uint32_t> dist(0, BabyBear::PRIME - 1); | ||
|
|
||
| BabyBear4 beta = random_ext_element<BabyBear, 4, 11>(); |
There was a problem hiding this comment.
In BM_FoldMatrix_BabyBear4, the challenge beta is generated using random_ext_element(), which relies on the global rng_global. However, the matrix mat is generated using a local_rng. For benchmark consistency and reproducibility, all random data within a benchmark function should be derived from the same local RNG. This is also how it's done in BM_FoldMatrix_BabyBear and BM_FoldMatrixCuda_BabyBear4.
std::array<BabyBear, 4> beta_coeffs;
for (auto& c : beta_coeffs) {
c = BabyBear(dist(local_rng));
}
BabyBear4 beta(beta_coeffs);
The `else if (err != cudaSuccess)` was always true since the success case was already handled above. Simplify to a plain `else`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Generate the challenge beta from local_rng instead of the global rng_global for reproducibility, matching the pattern used in BM_FoldMatrix_BabyBear and BM_FoldMatrixCuda_BabyBear4. Remove now-unused random_element/random_ext_element helpers and rng_global. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The CUDA fold benchmark was built but not included in the run_benchmarks custom target, so it would never run with the other benchmarks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new benchmarks for FRI folding, covering both CPU and CUDA implementations. It also includes several maintenance updates, such as suppressing pedantic warnings for 128-bit integers in CUDA compatibility headers, enhancing CUDA error reporting with explicit runtime errors, and minor refactoring of assertions in the Poseidon implementation. The review feedback suggests improving the maintainability of the CMake configuration by looping over benchmark targets for compiler options and reducing code duplication in the benchmark source files by utilizing C++ templates for different field types.
src/benches/CMakeLists.txt
Outdated
| if(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang") | ||
| target_compile_options(bench_field PRIVATE -O3 -march=native) | ||
| target_compile_options(transpose_benchmark PRIVATE -O3 -march=native) | ||
| target_compile_options(columnwise_dot_product PRIVATE -O3 -march=native) | ||
| target_compile_options(bench_fold_even_odd PRIVATE -O3 -march=native) | ||
| elseif(MSVC) | ||
| target_compile_options(bench_field PRIVATE /O2) | ||
| target_compile_options(transpose_benchmark PRIVATE /O2) | ||
| target_compile_options(columnwise_dot_product PRIVATE /O2) | ||
| target_compile_options(bench_fold_even_odd PRIVATE /O2) | ||
| endif() |
There was a problem hiding this comment.
To improve maintainability and avoid repeating target_compile_options for every benchmark, you can define a list of benchmark targets and loop over them to apply the compile options. This makes it easier to add or remove benchmarks in the future.
set(_bench_targets
bench_field
transpose_benchmark
columnwise_dot_product
bench_fold_even_odd
)
if(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang")
set(_compile_options -O3 -march=native)
elseif(MSVC)
set(_compile_options /O2)
endif()
if(_compile_options)
foreach(target ${_bench_targets})
target_compile_options(${target} PRIVATE ${_compile_options})
endforeach()
endif()
| // -------------------------------------------------------------------------- | ||
| // BabyBear (F = EF = BabyBear) | ||
| // -------------------------------------------------------------------------- | ||
| static void BM_FoldMatrix_BabyBear(benchmark::State& state) { | ||
| const size_t log_size = static_cast<size_t>(state.range(0)); | ||
| const size_t n = size_t(1) << log_size; | ||
| const size_t log_arity = 1; | ||
| const size_t log_height = log_size + log_arity; // total length = n * arity | ||
|
|
||
| // Generate random input: flat vector of length 2*n | ||
| std::mt19937_64 local_rng(n); | ||
| std::uniform_int_distribution<uint32_t> dist(0, BabyBear::PRIME - 1); | ||
|
|
||
| BabyBear beta(dist(local_rng)); | ||
| std::vector<BabyBear> mat(2 * n); | ||
| for (auto& v : mat) { | ||
| v = BabyBear(dist(local_rng)); | ||
| } | ||
|
|
||
| for (auto _ : state) { | ||
| auto result = TwoAdicFriFolding<BabyBear, BabyBear>::fold_matrix( | ||
| log_height, log_arity, beta, mat); | ||
| benchmark::DoNotOptimize(result.data()); | ||
| } | ||
| } | ||
|
|
||
| BENCHMARK(BM_FoldMatrix_BabyBear) | ||
| ->Args({12})->Args({14})->Args({16})->Args({18})->Args({20})->Args({22}) | ||
| ->Unit(benchmark::kMillisecond) | ||
| ->Iterations(10); | ||
|
|
||
| // -------------------------------------------------------------------------- | ||
| // BabyBear4 (F = BabyBear, EF = BinomialExtensionField<BabyBear, 4, 11>) | ||
| // -------------------------------------------------------------------------- | ||
| static void BM_FoldMatrix_BabyBear4(benchmark::State& state) { | ||
| const size_t log_size = static_cast<size_t>(state.range(0)); | ||
| const size_t n = size_t(1) << log_size; | ||
| const size_t log_arity = 1; | ||
| const size_t log_height = log_size + log_arity; | ||
|
|
||
| std::mt19937_64 local_rng(n); | ||
| std::uniform_int_distribution<uint32_t> dist(0, BabyBear::PRIME - 1); | ||
|
|
||
| std::array<BabyBear, 4> beta_coeffs; | ||
| for (auto& c : beta_coeffs) { | ||
| c = BabyBear(dist(local_rng)); | ||
| } | ||
| BabyBear4 beta(beta_coeffs); | ||
| std::vector<BabyBear4> mat(2 * n); | ||
| for (auto& v : mat) { | ||
| std::array<BabyBear, 4> coeffs; | ||
| for (auto& c : coeffs) { | ||
| c = BabyBear(dist(local_rng)); | ||
| } | ||
| v = BabyBear4(coeffs); | ||
| } | ||
|
|
||
| for (auto _ : state) { | ||
| auto result = TwoAdicFriFolding<BabyBear, BabyBear4>::fold_matrix( | ||
| log_height, log_arity, beta, mat); | ||
| benchmark::DoNotOptimize(result.data()); | ||
| } | ||
| } | ||
|
|
||
| BENCHMARK(BM_FoldMatrix_BabyBear4) | ||
| ->Args({12})->Args({14})->Args({16})->Args({18})->Args({20})->Args({22}) | ||
| ->Unit(benchmark::kMillisecond) | ||
| ->Iterations(10); |
There was a problem hiding this comment.
There's significant duplication between BM_FoldMatrix_BabyBear and BM_FoldMatrix_BabyBear4. You can avoid this by using a template for the benchmark function and BENCHMARK_TEMPLATE to register the different instantiations. This will make the code more concise and easier to maintain, especially if you add more field types in the future.
// Helper to generate random field elements.
template<typename T> T random_element(std::mt19937_64& rng);
template<>
BabyBear random_element<BabyBear>(std::mt19937_64& rng) {
std::uniform_int_distribution<uint32_t> dist(0, BabyBear::PRIME - 1);
return BabyBear(dist(rng));
}
template<>
BabyBear4 random_element<BabyBear4>(std::mt19937_64& rng) {
std::uniform_int_distribution<uint32_t> dist(0, BabyBear::PRIME - 1);
std::array<BabyBear, 4> coeffs;
for (auto& c : coeffs) {
c = BabyBear(dist(rng));
}
return BabyBear4(coeffs);
}
template <typename Challenge>
static void BM_FoldMatrix(benchmark::State& state) {
using Val = BabyBear;
const size_t log_size = static_cast<size_t>(state.range(0));
const size_t n = size_t(1) << log_size;
const size_t log_arity = 1;
const size_t log_height = log_size + log_arity;
std::mt19937_64 local_rng(n);
Challenge beta = random_element<Challenge>(local_rng);
std::vector<Challenge> mat(2 * n);
for (auto& v : mat) {
v = random_element<Challenge>(local_rng);
}
for (auto _ : state) {
auto result = TwoAdicFriFolding<Val, Challenge>::fold_matrix(
log_height, log_arity, beta, mat);
benchmark::DoNotOptimize(result.data());
}
}
BENCHMARK_TEMPLATE(BM_FoldMatrix, BabyBear)
->Args({12})->Args({14})->Args({16})->Args({18})->Args({20})->Args({22})
->Unit(benchmark::kMillisecond)
->Iterations(10);
BENCHMARK_TEMPLATE(BM_FoldMatrix, BabyBear4)
->Args({12})->Args({14})->Args({16})->Args({18})->Args({20})->Args({22})
->Unit(benchmark::kMillisecond)
->Iterations(10);Replace per-target compile_options repetition with a loop over a list of CPU benchmark name:source pairs. Adding a new benchmark now only requires appending one entry to the list. The run_benchmarks target is also built from the same unified list. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
No description provided.