Skip to content

Commit

Permalink
Merge branch 'master' into gd/issue_6946
Browse files Browse the repository at this point in the history
  • Loading branch information
guipublic authored Jan 14, 2025
2 parents a3eb219 + 1e64f8a commit fb962e0
Show file tree
Hide file tree
Showing 18 changed files with 337 additions and 113 deletions.
45 changes: 13 additions & 32 deletions .github/workflows/reports.yml
Original file line number Diff line number Diff line change
Expand Up @@ -300,17 +300,17 @@ jobs:
matrix:
include:
# - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-contracts, is_library: true }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-inner, take_average: true }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-tail, take_average: true }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-reset, take_average: true }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-private, take_average: true }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-public, take_average: true }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-merge, take_average: true }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-inner, num_runs: 5 }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-tail, num_runs: 5 }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-reset, num_runs: 5 }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-private, num_runs: 5 }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-public, num_runs: 5 }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-merge, num_runs: 5 }
#- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root-empty }
#- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-single-tx }
#- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-merge, take_average: true }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-root, take_average: true }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-merge, num_runs: 5 }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-root, num_runs: 5 }

name: External repo compilation and execution reports - ${{ matrix.project.repo }}/${{ matrix.project.path }}
steps:
Expand Down Expand Up @@ -344,41 +344,22 @@ jobs:
path: test-repo
ref: ${{ matrix.project.ref }}

- name: Generate compilation report without averages
- name: Generate compilation report
working-directory: ./test-repo/${{ matrix.project.path }}
if: ${{ !matrix.project.take_average }}
run: |
mv /home/runner/work/noir/noir/scripts/test_programs/compilation_report.sh ./compilation_report.sh
touch parse_time.sh
chmod +x parse_time.sh
cp /home/runner/work/noir/noir/scripts/test_programs/parse_time.sh ./parse_time.sh
./compilation_report.sh 1
- name: Generate compilation report with averages
working-directory: ./test-repo/${{ matrix.project.path }}
if: ${{ matrix.project.take_average }}
run: |
mv /home/runner/work/noir/noir/scripts/test_programs/compilation_report.sh ./compilation_report.sh
touch parse_time.sh
chmod +x parse_time.sh
cp /home/runner/work/noir/noir/scripts/test_programs/parse_time.sh ./parse_time.sh
./compilation_report.sh 1 1
./compilation_report.sh 1 ${{ matrix.project.num_runs }}
- name: Generate execution report without averages
- name: Generate execution report
working-directory: ./test-repo/${{ matrix.project.path }}
if: ${{ !matrix.project.is_library && !matrix.project.take_average }}
run: |
mv /home/runner/work/noir/noir/scripts/test_programs/execution_report.sh ./execution_report.sh
mv /home/runner/work/noir/noir/scripts/test_programs/parse_time.sh ./parse_time.sh
./execution_report.sh 1
- name: Generate execution report with averages
working-directory: ./test-repo/${{ matrix.project.path }}
if: ${{ !matrix.project.is_library && matrix.project.take_average }}
if: ${{ !matrix.project.is_library }}
run: |
mv /home/runner/work/noir/noir/scripts/test_programs/execution_report.sh ./execution_report.sh
mv /home/runner/work/noir/noir/scripts/test_programs/parse_time.sh ./parse_time.sh
./execution_report.sh 1 1
./execution_report.sh 1 ${{ matrix.project.num_runs }}
- name: Move compilation report
id: compilation_report
Expand Down
4 changes: 1 addition & 3 deletions compiler/noirc_evaluator/src/ssa/ir/dfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ use noirc_errors::call_stack::{CallStack, CallStackHelper, CallStackId};
use serde::{Deserialize, Serialize};
use serde_with::serde_as;
use serde_with::DisplayFromStr;
use tracing::warn;

/// The DataFlowGraph contains most of the actual data in a function including
/// its blocks, instructions, and values. This struct is largely responsible for
Expand Down Expand Up @@ -238,8 +237,7 @@ impl DataFlowGraph {
call_stack: CallStackId,
) -> InsertInstructionResult {
if !self.is_handled_by_runtime(&instruction) {
warn!("Attempted to insert instruction not handled by runtime: {instruction:?}");
return InsertInstructionResult::InstructionRemoved;
panic!("Attempted to insert instruction not handled by runtime: {instruction:?}");
}

match instruction.simplify(self, block, ctrl_typevars.clone(), call_stack) {
Expand Down
3 changes: 2 additions & 1 deletion compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -307,14 +307,15 @@ impl<'brillig> Context<'brillig> {
let old_results = dfg.instruction_results(id).to_vec();

// If a copy of this instruction exists earlier in the block, then reuse the previous results.
let runtime_is_brillig = dfg.runtime().is_brillig();
if let Some(cache_result) =
self.get_cached(dfg, dom, &instruction, *side_effects_enabled_var, block)
{
match cache_result {
CacheResult::Cached(cached) => {
// We track whether we may mutate MakeArray instructions before we deduplicate
// them but we still need to issue an extra inc_rc in case they're mutated afterward.
if matches!(instruction, Instruction::MakeArray { .. }) {
if runtime_is_brillig && matches!(instruction, Instruction::MakeArray { .. }) {
let value = *cached.last().unwrap();
let inc_rc = Instruction::IncrementRc { value };
let call_stack = dfg.get_instruction_call_stack_id(id);
Expand Down
10 changes: 6 additions & 4 deletions compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,12 @@ impl<'f> LoopInvariantContext<'f> {

// If we are hoisting a MakeArray instruction,
// we need to issue an extra inc_rc in case they are mutated afterward.
if matches!(
self.inserter.function.dfg[instruction_id],
Instruction::MakeArray { .. }
) {
if self.inserter.function.runtime().is_brillig()
&& matches!(
self.inserter.function.dfg[instruction_id],
Instruction::MakeArray { .. }
)
{
let result =
self.inserter.function.dfg.instruction_results(instruction_id)[0];
let inc_rc = Instruction::IncrementRc { value: result };
Expand Down
6 changes: 6 additions & 0 deletions compiler/noirc_frontend/src/elaborator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1322,9 +1322,15 @@ impl<'context> Elaborator<'context> {
let span = trait_impl.object_type.span;
self.declare_methods_on_struct(Some(trait_id), &mut trait_impl.methods, span);

let trait_visibility = self.interner.get_trait(trait_id).visibility;

let methods = trait_impl.methods.function_ids();
for func_id in &methods {
self.interner.set_function_trait(*func_id, self_type.clone(), trait_id);

// A trait impl method has the same visibility as its trait
let modifiers = self.interner.function_modifiers_mut(func_id);
modifiers.visibility = trait_visibility;
}

let trait_generics = trait_impl.resolved_trait_generics.clone();
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_frontend/src/elaborator/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ impl<'context> Elaborator<'context> {
this.interner.update_trait(*trait_id, |trait_def| {
trait_def.set_trait_bounds(resolved_trait_bounds);
trait_def.set_where_clause(where_clause);
trait_def.set_visibility(unresolved_trait.trait_def.visibility);
});

let methods = this.resolve_trait_methods(*trait_id, unresolved_trait);
Expand Down
7 changes: 4 additions & 3 deletions compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1228,9 +1228,10 @@ pub(crate) fn collect_trait_impl_items(
for item in std::mem::take(&mut trait_impl.items) {
match item.item.kind {
TraitImplItemKind::Function(mut impl_method) => {
// Regardless of what visibility was on the source code, treat it as public
// (a warning is produced during parsing for this)
impl_method.def.visibility = ItemVisibility::Public;
// Set the impl method visibility as temporarily private.
// Eventually when we find out what trait is this impl for we'll set it
// to the trait's visibility.
impl_method.def.visibility = ItemVisibility::Private;

let func_id = interner.push_empty_fn();
let location = Location::new(impl_method.span(), file_id);
Expand Down
7 changes: 6 additions & 1 deletion compiler/noirc_frontend/src/hir_def/traits.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use iter_extended::vecmap;
use rustc_hash::FxHashMap as HashMap;

use crate::ast::{Ident, NoirFunction};
use crate::ast::{Ident, ItemVisibility, NoirFunction};
use crate::hir::type_check::generics::TraitGenerics;
use crate::ResolvedGeneric;
use crate::{
Expand Down Expand Up @@ -66,6 +66,7 @@ pub struct Trait {
pub name: Ident,
pub generics: Generics,
pub location: Location,
pub visibility: ItemVisibility,

/// When resolving the types of Trait elements, all references to `Self` resolve
/// to this TypeVariable. Then when we check if the types of trait impl elements
Expand Down Expand Up @@ -160,6 +161,10 @@ impl Trait {
self.where_clause = where_clause;
}

pub fn set_visibility(&mut self, visibility: ItemVisibility) {
self.visibility = visibility;
}

pub fn find_method(&self, name: &str) -> Option<TraitMethodId> {
for (idx, method) in self.methods.iter().enumerate() {
if &method.name == name {
Expand Down
40 changes: 18 additions & 22 deletions compiler/noirc_frontend/src/node_interner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -729,6 +729,7 @@ impl NodeInterner {
crate_id: unresolved_trait.crate_id,
location: Location::new(unresolved_trait.trait_def.span, unresolved_trait.file_id),
generics,
visibility: ItemVisibility::Private,
self_type_typevar: TypeVariable::unbound(self.next_type_variable_id(), Kind::Normal),
methods: Vec::new(),
method_ids: unresolved_trait.method_ids.clone(),
Expand Down Expand Up @@ -2268,31 +2269,26 @@ impl Methods {
}

/// Iterate through each method, starting with the direct methods
pub fn iter(&self) -> impl Iterator<Item = (FuncId, &Option<Type>)> + '_ {
let trait_impl_methods = self.trait_impl_methods.iter().map(|m| (m.method, &m.typ));
let direct = self.direct.iter().copied().map(|func_id| {
let typ: &Option<Type> = &None;
(func_id, typ)
});
pub fn iter(&self) -> impl Iterator<Item = (FuncId, Option<&Type>, Option<TraitId>)> {
let trait_impl_methods =
self.trait_impl_methods.iter().map(|m| (m.method, m.typ.as_ref(), Some(m.trait_id)));
let direct = self.direct.iter().copied().map(|func_id| (func_id, None, None));
direct.chain(trait_impl_methods)
}

/// Select the 1 matching method with an object type matching `typ`
pub fn find_matching_method(
&self,
typ: &Type,
pub fn find_matching_methods<'a>(
&'a self,
typ: &'a Type,
has_self_param: bool,
interner: &NodeInterner,
) -> Option<FuncId> {
// When adding methods we always check they do not overlap, so there should be
// at most 1 matching method in this list.
for (method, method_type) in self.iter() {
interner: &'a NodeInterner,
) -> impl Iterator<Item = (FuncId, Option<TraitId>)> + 'a {
self.iter().filter_map(move |(method, method_type, trait_id)| {
if Self::method_matches(typ, has_self_param, method, method_type, interner) {
return Some(method);
Some((method, trait_id))
} else {
None
}
}

None
})
}

pub fn find_direct_method(
Expand All @@ -2302,7 +2298,7 @@ impl Methods {
interner: &NodeInterner,
) -> Option<FuncId> {
for method in &self.direct {
if Self::method_matches(typ, has_self_param, *method, &None, interner) {
if Self::method_matches(typ, has_self_param, *method, None, interner) {
return Some(*method);
}
}
Expand All @@ -2320,7 +2316,7 @@ impl Methods {

for trait_impl_method in &self.trait_impl_methods {
let method = trait_impl_method.method;
let method_type = &trait_impl_method.typ;
let method_type = trait_impl_method.typ.as_ref();
let trait_id = trait_impl_method.trait_id;

if Self::method_matches(typ, has_self_param, method, method_type, interner) {
Expand All @@ -2335,7 +2331,7 @@ impl Methods {
typ: &Type,
has_self_param: bool,
method: FuncId,
method_type: &Option<Type>,
method_type: Option<&Type>,
interner: &NodeInterner,
) -> bool {
match interner.function_meta(&method).typ.instantiate(interner).0 {
Expand Down
3 changes: 1 addition & 2 deletions noir_stdlib/src/collections/umap.nr
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,7 @@ impl<K, V, B> UHashMap<K, V, B> {
{
// docs:end:contains_key
/// Safety: unconstrained context
unsafe { self.get(key) }
.is_some()
unsafe { self.get(key) }.is_some()
}

// Returns true if the map contains no elements.
Expand Down
8 changes: 1 addition & 7 deletions test_programs/compilation_report.sh
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,9 @@ for dir in ${tests_to_profile[@]}; do
PACKAGE_NAME=$(basename $current_dir)
fi

NUM_RUNS=1
NUM_RUNS=$2
TOTAL_TIME=0

# Passing a second argument will take an average of five runs
# rather than
if [ "$2" == "1" ]; then
NUM_RUNS=5
fi

for ((i = 1; i <= NUM_RUNS; i++)); do
NOIR_LOG=trace NARGO_LOG_DIR=./tmp nargo compile --force --silence-warnings
done
Expand Down
6 changes: 1 addition & 5 deletions test_programs/execution_report.sh
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,8 @@ for dir in ${tests_to_profile[@]}; do
fi


NUM_RUNS=1
NUM_RUNS=$2
TOTAL_TIME=0

if [ "$2" == "1" ]; then
NUM_RUNS=5
fi

for ((i = 1; i <= NUM_RUNS; i++)); do
NOIR_LOG=trace NARGO_LOG_DIR=./tmp nargo execute --silence-warnings
Expand Down
Loading

0 comments on commit fb962e0

Please sign in to comment.