Skip to content
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

fix(experimental elaborator): Move code to declare trait impls earlier #5105

Merged
merged 10 commits into from
May 28, 2024
Merged
180 changes: 93 additions & 87 deletions .github/workflows/gates_report.yml
Original file line number Diff line number Diff line change
@@ -1,88 +1,94 @@
# name: Report gates diff

# on:
# push:
# branches:
# - master
# pull_request:

# jobs:
# build-nargo:
# runs-on: ubuntu-latest
# strategy:
# matrix:
# target: [x86_64-unknown-linux-gnu]

# steps:
# - name: Checkout Noir repo
# uses: actions/checkout@v4

# - name: Setup toolchain
# uses: dtolnay/rust-toolchain@1.74.1

# - uses: Swatinem/rust-cache@v2
# with:
# key: ${{ matrix.target }}
# cache-on-failure: true
# save-if: ${{ github.event_name != 'merge_group' }}

# - name: Build Nargo
# run: cargo build --package nargo_cli --release

# - name: Package artifacts
# run: |
# mkdir dist
# cp ./target/release/nargo ./dist/nargo
# 7z a -ttar -so -an ./dist/* | 7z a -si ./nargo-x86_64-unknown-linux-gnu.tar.gz

# - name: Upload artifact
# uses: actions/upload-artifact@v4
# with:
# name: nargo
# path: ./dist/*
# retention-days: 3


# compare_gas_reports:
# needs: [build-nargo]
# runs-on: ubuntu-latest
# permissions:
# pull-requests: write

# steps:
# - uses: actions/checkout@v4

# - name: Download nargo binary
# uses: actions/download-artifact@v4
# with:
# name: nargo
# path: ./nargo

# - name: Set nargo on PATH
# run: |
# nargo_binary="${{ github.workspace }}/nargo/nargo"
# chmod +x $nargo_binary
# echo "$(dirname $nargo_binary)" >> $GITHUB_PATH
# export PATH="$PATH:$(dirname $nargo_binary)"
# nargo -V

# - name: Generate gates report
# working-directory: ./test_programs
# run: |
# ./gates_report.sh
# mv gates_report.json ../gates_report.json
name: Report gates diff

on:
push:
branches:
- master
pull_request:

jobs:
build-nargo:
runs-on: ubuntu-latest
strategy:
matrix:
target: [x86_64-unknown-linux-gnu]

steps:
- name: Checkout Noir repo
uses: actions/checkout@v4

- name: Setup toolchain
uses: dtolnay/rust-toolchain@1.74.1

- uses: Swatinem/rust-cache@v2
with:
key: ${{ matrix.target }}
cache-on-failure: true
save-if: ${{ github.event_name != 'merge_group' }}

- name: Build Nargo
run: cargo build --package nargo_cli --release

- name: Package artifacts
run: |
mkdir dist
cp ./target/release/nargo ./dist/nargo
7z a -ttar -so -an ./dist/* | 7z a -si ./nargo-x86_64-unknown-linux-gnu.tar.gz

- name: Upload artifact
uses: actions/upload-artifact@v4
with:
name: nargo
path: ./dist/*
retention-days: 3


compare_gates_reports:
needs: [build-nargo]
runs-on: ubuntu-latest
permissions:
pull-requests: write

steps:
- uses: actions/checkout@v4

- name: Install `bb`
run: |
./scripts/install_bb.sh
echo "$HOME/.bb/" >> $GITHUB_PATH

- name: Download nargo binary
uses: actions/download-artifact@v4
with:
name: nargo
path: ./nargo

- name: Set nargo on PATH
run: |
nargo_binary="${{ github.workspace }}/nargo/nargo"
chmod +x $nargo_binary
echo "$(dirname $nargo_binary)" >> $GITHUB_PATH
export PATH="$PATH:$(dirname $nargo_binary)"
nargo -V

- name: Generate gates report
working-directory: ./test_programs
run: |
./rebuild.sh
./gates_report.sh
mv gates_report.json ../gates_report.json

# - name: Compare gates reports
# id: gates_diff
# uses: vezenovm/noir-gates-diff@acf12797860f237117e15c0d6e08d64253af52b6
# with:
# report: gates_report.json
# summaryQuantile: 0.9 # only display the 10% most significant circuit size diffs in the summary (defaults to 20%)

# - name: Add gates diff to sticky comment
# if: github.event_name == 'pull_request' || github.event_name == 'pull_request_target'
# uses: marocchino/sticky-pull-request-comment@v2
# with:
# # delete the comment in case changes no longer impact circuit sizes
# delete: ${{ !steps.gates_diff.outputs.markdown }}
# message: ${{ steps.gates_diff.outputs.markdown }}
- name: Compare gates reports
id: gates_diff
uses: vezenovm/noir-gates-diff@acf12797860f237117e15c0d6e08d64253af52b6
with:
report: gates_report.json
summaryQuantile: 0.9 # only display the 10% most significant circuit size diffs in the summary (defaults to 20%)

- name: Add gates diff to sticky comment
if: github.event_name == 'pull_request' || github.event_name == 'pull_request_target'
uses: marocchino/sticky-pull-request-comment@v2
with:
# delete the comment in case changes no longer impact circuit sizes
delete: ${{ !steps.gates_diff.outputs.markdown }}
message: ${{ steps.gates_diff.outputs.markdown }}
4 changes: 2 additions & 2 deletions acvm-repo/acvm/src/compiler/transformers/csat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@
// This optimization will search for combinations of terms which can be represented in a single assert-zero opcode
// Case 1 : qM * wL * wR + qL * wL + qR * wR + qO * wO + qC
// This polynomial does not require any further optimizations, it can be safely represented in one opcode
// ie a polynomial with 1 mul(bi-variate) term and 3 (univariate) terms where 2 of those terms match the bivariate term

Check warning on line 90 in acvm-repo/acvm/src/compiler/transformers/csat.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (bivariate)
// wL and wR, we can represent it in one opcode
// GENERALIZED for WIDTH: instead of the number 3, we use `WIDTH`
//
Expand Down Expand Up @@ -210,16 +210,16 @@
}
} else {
// No more usable elements left in the old opcode
opcode.linear_combinations = remaining_linear_terms;
break;
}
}
opcode.linear_combinations.extend(remaining_linear_terms);
jfecher marked this conversation as resolved.
Show resolved Hide resolved

// Constraint this intermediate_opcode to be equal to the temp variable by adding it into the IndexMap
// We need a unique name for our intermediate variable
// XXX: Another optimization, which could be applied in another algorithm
// If two opcodes have a large fan-in/out and they share a few common terms, then we should create intermediate variables for them
// Do some sort of subset matching algorithm for this on the terms of the polynomial

let inter_var = Self::get_or_create_intermediate_vars(
intermediate_variables,
intermediate_opcode,
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_evaluator/src/ssa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ pub(crate) fn optimize_into_acir(
.run_pass(Ssa::defunctionalize, "After Defunctionalization:")
.run_pass(Ssa::remove_paired_rc, "After Removing Paired rc_inc & rc_decs:")
.run_pass(Ssa::inline_functions, "After Inlining:")
.run_pass(Ssa::resolve_is_unconstrained, "After Resolving IsUnconstrained:")
// Run mem2reg with the CFG separated into blocks
.run_pass(Ssa::mem2reg, "After Mem2Reg:")
.run_pass(Ssa::as_slice_optimization, "After `as_slice` optimization")
Expand Down
41 changes: 36 additions & 5 deletions compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -614,13 +614,44 @@ impl AcirContext {
expr.push_multiplication_term(FieldElement::one(), lhs_witness, rhs_witness);
self.add_data(AcirVarData::Expr(expr))
}
(
AcirVarData::Expr(_) | AcirVarData::Witness(_),
AcirVarData::Expr(_) | AcirVarData::Witness(_),
) => {
(AcirVarData::Expr(expression), AcirVarData::Witness(witness))
| (AcirVarData::Witness(witness), AcirVarData::Expr(expression))
if expression.is_linear() =>
{
let mut expr = Expression::default();
for term in expression.linear_combinations.iter() {
expr.push_multiplication_term(term.0, term.1, witness);
}
expr.push_addition_term(expression.q_c, witness);
self.add_data(AcirVarData::Expr(expr))
}
(AcirVarData::Expr(lhs_expr), AcirVarData::Expr(rhs_expr)) => {
let degree_one = if lhs_expr.is_linear() && rhs_expr.is_degree_one_univariate() {
Some((lhs_expr, rhs_expr))
} else if rhs_expr.is_linear() && lhs_expr.is_degree_one_univariate() {
Some((rhs_expr, lhs_expr))
} else {
None
};
if let Some((lin, univariate)) = degree_one {
let mut expr = Expression::default();
let rhs_term = univariate.linear_combinations[0];
for term in lin.linear_combinations.iter() {
expr.push_multiplication_term(term.0 * rhs_term.0, term.1, rhs_term.1);
}
expr.push_addition_term(lin.q_c * rhs_term.0, rhs_term.1);
expr.sort();
expr = expr.add_mul(univariate.q_c, &lin);
self.add_data(AcirVarData::Expr(expr))
} else {
let lhs = self.get_or_create_witness_var(lhs)?;
let rhs = self.get_or_create_witness_var(rhs)?;
self.mul_var(lhs, rhs)?
}
}
_ => {
let lhs = self.get_or_create_witness_var(lhs)?;
let rhs = self.get_or_create_witness_var(rhs)?;

self.mul_var(lhs, rhs)?
}
};
Expand Down
6 changes: 5 additions & 1 deletion compiler/noirc_evaluator/src/ssa/ir/instruction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ pub(crate) enum Intrinsic {
FromField,
AsField,
AsWitness,
IsUnconstrained,
}

impl std::fmt::Display for Intrinsic {
Expand All @@ -89,6 +90,7 @@ impl std::fmt::Display for Intrinsic {
Intrinsic::FromField => write!(f, "from_field"),
Intrinsic::AsField => write!(f, "as_field"),
Intrinsic::AsWitness => write!(f, "as_witness"),
Intrinsic::IsUnconstrained => write!(f, "is_unconstrained"),
}
}
}
Expand Down Expand Up @@ -116,7 +118,8 @@ impl Intrinsic {
| Intrinsic::SliceRemove
| Intrinsic::StrAsBytes
| Intrinsic::FromField
| Intrinsic::AsField => false,
| Intrinsic::AsField
| Intrinsic::IsUnconstrained => false,

// Some black box functions have side-effects
Intrinsic::BlackBox(func) => matches!(func, BlackBoxFunc::RecursiveAggregation),
Expand Down Expand Up @@ -145,6 +148,7 @@ impl Intrinsic {
"from_field" => Some(Intrinsic::FromField),
"as_field" => Some(Intrinsic::AsField),
"as_witness" => Some(Intrinsic::AsWitness),
"is_unconstrained" => Some(Intrinsic::IsUnconstrained),
other => BlackBoxFunc::lookup(other).map(Intrinsic::BlackBox),
}
}
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,7 @@ pub(super) fn simplify_call(
SimplifyResult::SimplifiedToInstruction(instruction)
}
Intrinsic::AsWitness => SimplifyResult::None,
Intrinsic::IsUnconstrained => SimplifyResult::None,
}
}

Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_evaluator/src/ssa/opt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,6 @@ mod rc;
mod remove_bit_shifts;
mod remove_enable_side_effects;
mod remove_if_else;
mod resolve_is_unconstrained;
mod simplify_cfg;
mod unrolling;
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,8 @@ impl Context {
| Intrinsic::FromField
| Intrinsic::AsField
| Intrinsic::AsSlice
| Intrinsic::AsWitness => false,
| Intrinsic::AsWitness
| Intrinsic::IsUnconstrained => false,
},

// We must assume that functions contain a side effect as we cannot inspect more deeply.
Expand Down
3 changes: 2 additions & 1 deletion compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ fn slice_capacity_change(
| Intrinsic::BlackBox(_)
| Intrinsic::FromField
| Intrinsic::AsField
| Intrinsic::AsWitness => SizeChange::None,
| Intrinsic::AsWitness
| Intrinsic::IsUnconstrained => SizeChange::None,
}
}
56 changes: 56 additions & 0 deletions compiler/noirc_evaluator/src/ssa/opt/resolve_is_unconstrained.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
use crate::ssa::{
ir::{
function::{Function, RuntimeType},
instruction::{Instruction, Intrinsic},
types::Type,
value::Value,
},
ssa_gen::Ssa,
};
use acvm::FieldElement;
use fxhash::FxHashSet as HashSet;

impl Ssa {
/// An SSA pass to find any calls to `Intrinsic::IsUnconstrained` and replacing any uses of the result of the intrinsic
/// with the resolved boolean value.
/// Note that this pass must run after the pass that does runtime separation, since in SSA generation an ACIR function can end up targeting brillig.
#[tracing::instrument(level = "trace", skip(self))]
pub(crate) fn resolve_is_unconstrained(mut self) -> Self {
for func in self.functions.values_mut() {
replace_is_unconstrained_result(func);
}
self
}
}

fn replace_is_unconstrained_result(func: &mut Function) {
let mut is_unconstrained_calls = HashSet::default();
// Collect all calls to is_unconstrained
for block_id in func.reachable_blocks() {
for &instruction_id in func.dfg[block_id].instructions() {
let target_func = match &func.dfg[instruction_id] {
Instruction::Call { func, .. } => *func,
_ => continue,
};

if let Value::Intrinsic(Intrinsic::IsUnconstrained) = &func.dfg[target_func] {
is_unconstrained_calls.insert(instruction_id);
}
}
}

for instruction_id in is_unconstrained_calls {
let call_returns = func.dfg.instruction_results(instruction_id);
let original_return_id = call_returns[0];

// We replace the result with a fresh id. This will be unused, so the DIE pass will remove the leftover intrinsic call.
func.dfg.replace_result(instruction_id, original_return_id);

let is_within_unconstrained = func.dfg.make_constant(
FieldElement::from(matches!(func.runtime(), RuntimeType::Brillig)),
Type::bool(),
);
// Replace all uses of the original return value with the constant
func.dfg.set_value_from_id(original_return_id, is_within_unconstrained);
}
}
Loading
Loading