-
Notifications
You must be signed in to change notification settings - Fork 234
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
feat: Avoiding redundant computation in PG #5844
Changes from 7 commits
dea0b42
9e89bfd
84d908f
6c26d53
c739afa
e38871e
2f09890
1ae27c0
182b8c8
9fe8e0f
30cb2fb
205d917
d218976
982bd6d
e9f2578
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -241,6 +241,24 @@ static constexpr auto create_protogalaxy_tuple_of_tuples_of_univariates() | |
} | ||
} | ||
|
||
/** | ||
* @brief Recursive utility function to construct a container for the subrelation accumulators of Protogalaxy folding. | ||
* @details Differs from the non-optimised method by using optimised univariates that skip redundant computation | ||
*/ | ||
template <typename Tuple, size_t NUM_INSTANCES, size_t Index = 0> | ||
static constexpr auto create_optimised_protogalaxy_tuple_of_tuples_of_univariates() | ||
{ | ||
if constexpr (Index >= std::tuple_size<Tuple>::value) { | ||
return std::tuple<>{}; // Return empty when reach end of the tuple | ||
} else { | ||
using UnivariateTuple = typename std::tuple_element_t<Index, Tuple>:: | ||
template OptimisedProtogalaxyTupleOfUnivariatesOverSubrelations<NUM_INSTANCES>; | ||
return std::tuple_cat( | ||
std::tuple<UnivariateTuple>{}, | ||
create_optimised_protogalaxy_tuple_of_tuples_of_univariates<Tuple, NUM_INSTANCES, Index + 1>()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we not overload in some way instead of duplicating code? |
||
} | ||
} | ||
|
||
/** | ||
* @brief Recursive utility function to construct a container for the subrelation accumulators of sumcheck proving. | ||
* @details The size of the outer tuple is equal to the number of relations. Each relation contributes an inner tuple of | ||
|
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,7 +53,7 @@ TEST(Protogalaxy, CombinerOn2Instances) | |
ProverInstances instances{ instance_data }; | ||
instances.alphas.fill(bb::Univariate<FF, 12>(FF(0))); // focus on the arithmetic relation only | ||
auto pow_polynomial = PowPolynomial(std::vector<FF>{ 2 }); | ||
auto result = prover.compute_combiner(instances, pow_polynomial); | ||
auto result = prover.compute_combiner</*disable_optimisation=*/true>(instances, pow_polynomial); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think it would be great to have these tests run for the optimised version as well There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can use the python script to generate the correct combiner values There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but they actually shouldn't be modified if I understand correctly, just some of them skipped There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the values |
||
auto expected_result = Univariate<FF, 12>(std::array<FF, 12>{ | ||
87706, | ||
13644570, | ||
|
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -879,6 +879,13 @@ class GoblinTranslatorFlavor { | |
* @brief A container for univariates used during sumcheck. | ||
*/ | ||
template <size_t LENGTH> using ProverUnivariates = AllEntities<bb::Univariate<FF, LENGTH>>; | ||
/** | ||
* @brief A container for univariates used during Protogalaxy folding and sumcheck with some of the computation | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. translator doesn't have anything to do with Protogalaxy (same for eccvm and avm) so I wonder whether this should be here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, removed |
||
* optmistically ignored | ||
* @details During folding and sumcheck, the prover evaluates the relations on these univariates. | ||
*/ | ||
template <size_t LENGTH, size_t SKIP_COUNT> | ||
using OptimisedProverUnivariates = AllEntities<bb::Univariate<FF, LENGTH, 0, SKIP_COUNT>>; | ||
|
||
/** | ||
* @brief A container for univariates produced during the hot loop in sumcheck. | ||
|
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.
Not needed?