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

feat: Sync from noir #8435

Merged
merged 10 commits into from
Sep 9, 2024
4 changes: 2 additions & 2 deletions noir/noir-repo/aztec_macros/src/utils/hir_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ pub fn inject_fn(
let trait_id = None;
items.functions.push(UnresolvedFunctions { file_id, functions, trait_id, self_type: None });

let mut errors = Elaborator::elaborate(context, *crate_id, items, None, false);
let mut errors = Elaborator::elaborate(context, *crate_id, items, None);
errors.retain(|(error, _)| !CustomDiagnostic::from(error).is_warning());

if !errors.is_empty() {
Expand Down Expand Up @@ -241,7 +241,7 @@ pub fn inject_global(
let mut items = CollectedItems::default();
items.globals.push(UnresolvedGlobal { file_id, module_id, global_id, stmt_def: global });

let _errors = Elaborator::elaborate(context, *crate_id, items, None, false);
let _errors = Elaborator::elaborate(context, *crate_id, items, None);
}

pub fn fully_qualified_note_path(context: &HirContext, note_id: StructId) -> Option<String> {
Expand Down
5 changes: 0 additions & 5 deletions noir/noir-repo/compiler/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,6 @@ pub struct CompileOptions {
#[arg(long, hide = true)]
pub show_artifact_paths: bool,

/// Temporary flag to enable the experimental arithmetic generics feature
#[arg(long, hide = true)]
pub arithmetic_generics: bool,

/// Flag to turn off the compiler check for under constrained values.
/// Warning: This can improve compilation speed but can also lead to correctness errors.
/// This check should always be run on production code.
Expand Down Expand Up @@ -289,7 +285,6 @@ pub fn check_crate(
crate_id,
context,
options.debug_comptime_in_file.as_deref(),
options.arithmetic_generics,
error_on_unused_imports,
macros,
);
Expand Down
115 changes: 107 additions & 8 deletions noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,12 @@ impl<'f> PerFunctionContext<'f> {
self.last_loads.get(store_address).is_none()
};

if remove_load && !is_reference_param {
let is_reference_alias = block
.expressions
.get(store_address)
.map_or(false, |expression| matches!(expression, Expression::Dereference(_)));

if remove_load && !is_reference_param && !is_reference_alias {
self.instructions_to_remove.insert(*store_instruction);
}
}
Expand Down Expand Up @@ -286,19 +291,19 @@ impl<'f> PerFunctionContext<'f> {
} else {
references.mark_value_used(address, self.inserter.function);

let expression = if let Some(expression) = references.expressions.get(&result) {
expression.clone()
} else {
references.expressions.insert(result, Expression::Other(result));
Expression::Other(result)
};
if let Some(aliases) = references.aliases.get_mut(&expression) {
let expression =
references.expressions.entry(result).or_insert(Expression::Other(result));
// Make sure this load result is marked an alias to itself
if let Some(aliases) = references.aliases.get_mut(expression) {
// If we have an alias set, add to the set
aliases.insert(result);
} else {
// Otherwise, create a new alias set containing just the load result
references
.aliases
.insert(Expression::Other(result), AliasSet::known(result));
}
// Mark that we know a load result is equivalent to the address of a load.
references.set_known_value(result, address);

self.last_loads.insert(address, (instruction, block_id));
Expand Down Expand Up @@ -789,4 +794,98 @@ mod tests {
// We expect the last eq to be optimized out
assert_eq!(b1_instructions.len(), 0);
}

#[test]
fn keep_store_to_alias_in_loop_block() {
// This test makes sure the instruction `store Field 2 at v5` in b2 remains after mem2reg.
// Although the only instruction on v5 is a lone store without any loads,
// v5 is an alias of the reference v0 which is stored in v2.
// This test makes sure that we are not inadvertently removing stores to aliases across blocks.
//
// acir(inline) fn main f0 {
// b0():
// v0 = allocate
// store Field 0 at v0
// v2 = allocate
// store v0 at v2
// jmp b1(Field 0)
// b1(v3: Field):
// v4 = eq v3, Field 0
// jmpif v4 then: b2, else: b3
// b2():
// v5 = load v2
// store Field 2 at v5
// v8 = add v3, Field 1
// jmp b1(v8)
// b3():
// v9 = load v0
// v10 = eq v9, Field 2
// constrain v9 == Field 2
// v11 = load v2
// v12 = load v10
// v13 = eq v12, Field 2
// constrain v11 == Field 2
// return
// }
let main_id = Id::test_new(0);
let mut builder = FunctionBuilder::new("main".into(), main_id);

let v0 = builder.insert_allocate(Type::field());
let zero = builder.numeric_constant(0u128, Type::field());
builder.insert_store(v0, zero);

let v2 = builder.insert_allocate(Type::field());
// Construct alias
builder.insert_store(v2, v0);
let v2_type = builder.current_function.dfg.type_of_value(v2);
assert!(builder.current_function.dfg.value_is_reference(v2));

let b1 = builder.insert_block();
builder.terminate_with_jmp(b1, vec![zero]);

// Loop header
builder.switch_to_block(b1);
let v3 = builder.add_block_parameter(b1, Type::field());
let is_zero = builder.insert_binary(v3, BinaryOp::Eq, zero);

let b2 = builder.insert_block();
let b3 = builder.insert_block();
builder.terminate_with_jmpif(is_zero, b2, b3);

// Loop body
builder.switch_to_block(b2);
let v5 = builder.insert_load(v2, v2_type.clone());
let two = builder.numeric_constant(2u128, Type::field());
builder.insert_store(v5, two);
let one = builder.numeric_constant(1u128, Type::field());
let v3_plus_one = builder.insert_binary(v3, BinaryOp::Add, one);
builder.terminate_with_jmp(b1, vec![v3_plus_one]);

builder.switch_to_block(b3);
let v9 = builder.insert_load(v0, Type::field());
let _ = builder.insert_binary(v9, BinaryOp::Eq, two);

builder.insert_constrain(v9, two, None);
let v11 = builder.insert_load(v2, v2_type);
let v12 = builder.insert_load(v11, Type::field());
let _ = builder.insert_binary(v12, BinaryOp::Eq, two);

builder.insert_constrain(v11, two, None);
builder.terminate_with_return(vec![]);

let ssa = builder.finish();

// We expect the same result as above.
let ssa = ssa.mem2reg();

let main = ssa.main();
assert_eq!(main.reachable_blocks().len(), 4);

// The store from the original SSA should remain
assert_eq!(count_stores(main.entry_block(), &main.dfg), 2);
assert_eq!(count_stores(b2, &main.dfg), 1);

assert_eq!(count_loads(b2, &main.dfg), 1);
assert_eq!(count_loads(b3, &main.dfg), 3);
}
}
1 change: 1 addition & 0 deletions noir/noir-repo/compiler/noirc_frontend/src/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ mod traits;
mod type_alias;
mod visitor;

pub use visitor::AttributeTarget;
pub use visitor::Visitor;

pub use expression::*;
Expand Down
55 changes: 50 additions & 5 deletions noir/noir-repo/compiler/noirc_frontend/src/ast/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use crate::{
QuotedTypeId,
},
parser::{Item, ItemKind, ParsedSubModule},
token::{SecondaryAttribute, Tokens},
token::{CustomAtrribute, SecondaryAttribute, Tokens},
ParsedModule, QuotedType,
};

Expand All @@ -26,6 +26,13 @@ use super::{
UnresolvedTypeExpression,
};

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub enum AttributeTarget {
Module,
Struct,
Function,
}

/// Implements the [Visitor pattern](https://en.wikipedia.org/wiki/Visitor_pattern) for Noir's AST.
///
/// In this implementation, methods must return a bool:
Expand Down Expand Up @@ -433,7 +440,15 @@ pub trait Visitor {
true
}

fn visit_secondary_attribute(&mut self, _: &SecondaryAttribute, _: Span) {}
fn visit_secondary_attribute(
&mut self,
_: &SecondaryAttribute,
_target: AttributeTarget,
) -> bool {
true
}

fn visit_custom_attribute(&mut self, _: &CustomAtrribute, _target: AttributeTarget) {}
}

impl ParsedModule {
Expand Down Expand Up @@ -484,14 +499,18 @@ impl Item {
module_declaration.accept(self.span, visitor);
}
ItemKind::InnerAttribute(attribute) => {
attribute.accept(self.span, visitor);
attribute.accept(AttributeTarget::Module, visitor);
}
}
}
}

impl ParsedSubModule {
pub fn accept(&self, span: Span, visitor: &mut impl Visitor) {
for attribute in &self.outer_attributes {
attribute.accept(AttributeTarget::Module, visitor);
}

if visitor.visit_parsed_submodule(self, span) {
self.accept_children(visitor);
}
Expand All @@ -510,6 +529,10 @@ impl NoirFunction {
}

pub fn accept_children(&self, visitor: &mut impl Visitor) {
for attribute in self.secondary_attributes() {
attribute.accept(AttributeTarget::Function, visitor);
}

for param in &self.def.parameters {
param.typ.accept(visitor);
}
Expand Down Expand Up @@ -674,6 +697,10 @@ impl NoirStruct {
}

pub fn accept_children(&self, visitor: &mut impl Visitor) {
for attribute in &self.attributes {
attribute.accept(AttributeTarget::Struct, visitor);
}

for (_name, unresolved_type) in &self.fields {
unresolved_type.accept(visitor);
}
Expand All @@ -694,6 +721,10 @@ impl NoirTypeAlias {

impl ModuleDeclaration {
pub fn accept(&self, span: Span, visitor: &mut impl Visitor) {
for attribute in &self.outer_attributes {
attribute.accept(AttributeTarget::Module, visitor);
}

visitor.visit_module_declaration(self, span);
}
}
Expand Down Expand Up @@ -1295,8 +1326,22 @@ impl Pattern {
}

impl SecondaryAttribute {
pub fn accept(&self, span: Span, visitor: &mut impl Visitor) {
visitor.visit_secondary_attribute(self, span);
pub fn accept(&self, target: AttributeTarget, visitor: &mut impl Visitor) {
if visitor.visit_secondary_attribute(self, target) {
self.accept_children(target, visitor);
}
}

pub fn accept_children(&self, target: AttributeTarget, visitor: &mut impl Visitor) {
if let SecondaryAttribute::Custom(custom) = self {
custom.accept(target, visitor);
}
}
}

impl CustomAtrribute {
pub fn accept(&self, target: AttributeTarget, visitor: &mut impl Visitor) {
visitor.visit_custom_attribute(self, target);
}
}

Expand Down
45 changes: 34 additions & 11 deletions noir/noir-repo/compiler/noirc_frontend/src/elaborator/comptime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,41 @@ impl<'context> Elaborator<'context> {
/// Elaborate an expression from the middle of a comptime scope.
/// When this happens we require additional information to know
/// what variables should be in scope.
pub fn elaborate_item_from_comptime<'a, T>(
pub fn elaborate_item_from_comptime_in_function<'a, T>(
&'a mut self,
current_function: Option<FuncId>,
f: impl FnOnce(&mut Elaborator<'a>) -> T,
) -> T {
self.elaborate_item_from_comptime(f, |elaborator| {
if let Some(function) = current_function {
let meta = elaborator.interner.function_meta(&function);
elaborator.current_item = Some(DependencyId::Function(function));
elaborator.crate_id = meta.source_crate;
elaborator.local_module = meta.source_module;
elaborator.file = meta.source_file;
elaborator.introduce_generics_into_scope(meta.all_generics.clone());
}
})
}

pub fn elaborate_item_from_comptime_in_module<'a, T>(
&'a mut self,
module: ModuleId,
file: FileId,
f: impl FnOnce(&mut Elaborator<'a>) -> T,
) -> T {
self.elaborate_item_from_comptime(f, |elaborator| {
elaborator.current_item = None;
elaborator.crate_id = module.krate;
elaborator.local_module = module.local_id;
elaborator.file = file;
})
}

fn elaborate_item_from_comptime<'a, T>(
&'a mut self,
f: impl FnOnce(&mut Elaborator<'a>) -> T,
setup: impl FnOnce(&mut Elaborator<'a>),
) -> T {
// Create a fresh elaborator to ensure no state is changed from
// this elaborator
Expand All @@ -64,21 +95,13 @@ impl<'context> Elaborator<'context> {
self.def_maps,
self.crate_id,
self.debug_comptime_in_file,
self.enable_arithmetic_generics,
self.interpreter_call_stack.clone(),
);

elaborator.function_context.push(FunctionContext::default());
elaborator.scopes.start_function();

if let Some(function) = current_function {
let meta = elaborator.interner.function_meta(&function);
elaborator.current_item = Some(DependencyId::Function(function));
elaborator.crate_id = meta.source_crate;
elaborator.local_module = meta.source_module;
elaborator.file = meta.source_file;
elaborator.introduce_generics_into_scope(meta.all_generics.clone());
}
setup(&mut elaborator);

elaborator.populate_scope_from_comptime_scopes();

Expand Down Expand Up @@ -352,7 +375,7 @@ impl<'context> Elaborator<'context> {
}
}

fn add_item(
pub(crate) fn add_item(
&mut self,
item: TopLevelStatement,
generated_items: &mut CollectedItems,
Expand Down
Loading
Loading