Skip to content

Commit

Permalink
fix(bb): eliminate recursion in accumulate* (#8205)
Browse files Browse the repository at this point in the history
Jean is working on the AVM recursive verifier and he found that these
functions were executed recursively (the compiler was indeed generating
recursive calls) and causing a stack overflow. This fixes that.
* ~~Also fixed `accumulate_relation_evaluations_without_skipping` which
was only not skipping the first relation.~~ Tests fail with the fix,
I've added a comment.
* I also made some params `const&`. IIUC they were being copied before
which can be massive for the type `AllValues`. Not sure about that but
you might want to check the callers, etc.
  • Loading branch information
fcarreiro authored Aug 27, 2024
1 parent 61d6f25 commit 47e83fa
Showing 1 changed file with 32 additions and 20 deletions.
52 changes: 32 additions & 20 deletions barretenberg/cpp/src/barretenberg/relations/utils.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
#include "barretenberg/polynomials/pow.hpp"
#include "barretenberg/relations/relation_parameters.hpp"
#include <tuple>
#include <utility>

namespace bb {

template <typename Flavor> class RelationUtils {
Expand Down Expand Up @@ -138,6 +140,14 @@ template <typename Flavor> class RelationUtils {
}
}

// This is an simpler, iterative version of constexpr_for.
// Replace it with constexpr_for once that one is iterative.
template <size_t N, typename F> static constexpr void iterative_constexpr_for(F&& f)
{
auto seq = std::make_index_sequence<N>{};
[&]<size_t... I>(std::index_sequence<I...>) { (f.template operator()<I>(), ...); }(seq);
}

/**
* @brief Calculate the contribution of each relation to the expected value of the full Honk relation.
*
Expand All @@ -147,23 +157,18 @@ template <typename Flavor> class RelationUtils {
* relation. This value is checked against the final value of the target total sum (called sigma_0 in the
* thesis).
*/
template <typename Parameters, size_t relation_idx = 0>
template <typename Parameters>
// TODO(#224)(Cody): Input should be an array?
inline static void accumulate_relation_evaluations_without_skipping(PolynomialEvaluations evaluations,
inline static void accumulate_relation_evaluations_without_skipping(const PolynomialEvaluations& evaluations,
RelationEvaluations& relation_evaluations,
const Parameters& relation_parameters,
const FF& partial_evaluation_result)
{
using Relation = std::tuple_element_t<relation_idx, Relations>;

Relation::accumulate(
std::get<relation_idx>(relation_evaluations), evaluations, relation_parameters, partial_evaluation_result);

// Repeat for the next relation.
if constexpr (relation_idx + 1 < NUM_RELATIONS) {
accumulate_relation_evaluations<Parameters, relation_idx + 1>(
iterative_constexpr_for<NUM_RELATIONS>([&]<size_t rel_index>() {
// FIXME: You wan't /*consider_skipping=*/false here, but tests need to be fixed.
accumulate_single_relation<Parameters, rel_index, /*consider_skipping=*/true>(
evaluations, relation_evaluations, relation_parameters, partial_evaluation_result);
}
});
}

/**
Expand All @@ -175,17 +180,30 @@ template <typename Flavor> class RelationUtils {
* relation. This value is checked against the final value of the target total sum (called sigma_0 in the
* thesis).
*/
template <typename Parameters, size_t relation_idx = 0>
template <typename Parameters>
// TODO(#224)(Cody): Input should be an array?
inline static void accumulate_relation_evaluations(PolynomialEvaluations evaluations,
inline static void accumulate_relation_evaluations(const PolynomialEvaluations& evaluations,
RelationEvaluations& relation_evaluations,
const Parameters& relation_parameters,
const FF& partial_evaluation_result)
{
iterative_constexpr_for<NUM_RELATIONS>([&]<size_t rel_index>() {
accumulate_single_relation<Parameters, rel_index>(
evaluations, relation_evaluations, relation_parameters, partial_evaluation_result);
});
}

template <typename Parameters, size_t relation_idx, bool consider_skipping = true>
inline static void accumulate_single_relation(const PolynomialEvaluations& evaluations,
RelationEvaluations& relation_evaluations,
const Parameters& relation_parameters,
const FF& partial_evaluation_result)
{
using Relation = std::tuple_element_t<relation_idx, Relations>;

// Check if the relation is skippable to speed up accumulation
if constexpr (!isSkippable<Relation, decltype(evaluations)> || !std::is_same_v<FF, bb::fr>) {
if constexpr (!consider_skipping || !isSkippable<Relation, decltype(evaluations)> ||
!std::is_same_v<FF, bb::fr>) {
// If not, accumulate normally
Relation::accumulate(std::get<relation_idx>(relation_evaluations),
evaluations,
Expand All @@ -200,12 +218,6 @@ template <typename Flavor> class RelationUtils {
partial_evaluation_result);
}
}

// Repeat for the next relation.
if constexpr (relation_idx + 1 < NUM_RELATIONS) {
accumulate_relation_evaluations<Parameters, relation_idx + 1>(
evaluations, relation_evaluations, relation_parameters, partial_evaluation_result);
}
}

/**
Expand Down

0 comments on commit 47e83fa

Please sign in to comment.