-
Notifications
You must be signed in to change notification settings - Fork 587
feat(avm)!: smaller precomputed trace #20144
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
base: merge-train/avm
Are you sure you want to change the base?
Conversation
| auto index_of_max_end_index = [](const auto& polys) { | ||
| size_t max_idx = 0; | ||
| size_t max_end_idx = polys[0].end_index(); | ||
| for (size_t idx = 0; const auto& poly : polys) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fcarreiro Maybe this way of iterating is a bit dangerous. Maybe we do not have the guarantee that the iteration is following the indices of the span.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can use these functions
// this assumes non-empty, returns an iterator
auto it = std::max_element(nums.begin(), nums.end(), [](int a, int b) {
return a < b; // Your custom comparator logic
});
// retrieves the index
size_t index = std::distance(nums.begin(), it);
Otherwise I would do a good old for loop. Spans have size(), and they are contiguous.
fcarreiro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some doubts about the PIL changes. I think that only lookups into precomputed or PIs should use idx. Shouldn't the rest use the execution clk? In particular think of memory. In simulation the clk manager (or whatever the name was) links it to execution.
| auto index_of_max_end_index = [](const auto& polys) { | ||
| size_t max_idx = 0; | ||
| size_t max_end_idx = polys[0].end_index(); | ||
| for (size_t idx = 0; const auto& poly : polys) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can use these functions
// this assumes non-empty, returns an iterator
auto it = std::max_element(nums.begin(), nums.end(), [](int a, int b) {
return a < b; // Your custom comparator logic
});
// retrieves the index
size_t index = std::distance(nums.begin(), it);
Otherwise I would do a good old for loop. Spans have size(), and they are contiguous.
barretenberg/cpp/src/barretenberg/vm2/tracegen/precomputed_trace.cpp
Outdated
Show resolved
Hide resolved
barretenberg/cpp/src/barretenberg/vm2/tracegen/precomputed_trace.cpp
Outdated
Show resolved
Hide resolved
barretenberg/cpp/src/barretenberg/vm2/tracegen/precomputed_trace.hpp
Outdated
Show resolved
Hide resolved
@fcarreiro You are absolutely right. I think I overlooked the virtual trace of execution. I will review all of these changes more carefully. |
18b28ac to
82d36b4
Compare
82d36b4 to
8d4560e
Compare
|
@federicobarbacovi @iakovenkos Could you please have a look at the changes in the files: prover.cpp, verifier.cpp, recursive_verifier.cpp? |
| idx++; | ||
| } | ||
|
|
||
| PolynomialBatcher polynomial_batcher(ProvingKey::circuit_size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iakovenkos Is this correct that we should not change the size of polynomial_batcher ?
| polynomial_batcher.set_unshifted(RefVector{ batched_unshifted }); | ||
| polynomial_batcher.set_to_be_shifted_by_one(RefVector{ batched_shifted }); | ||
|
|
||
| const size_t circuit_dyadic_size = numeric::round_up_power_2(batched_unshifted.end_index()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iakovenkos Is this what you expected?
This PR performs the following changes (kudo to @fcarreiro for suggesting this):
Performance improvements
3sto ~1.3son 32 cores.