Skip to content

Commit

Permalink
fix: Add missing is_empty check for enums (noir-lang/noir#7257)
Browse files Browse the repository at this point in the history
feat(experimental): Implement enum tag constants (noir-lang/noir#7183)
fix(unrolling): Fetch original bytecode size from the original function (noir-lang/noir#7253)
fix(ssa): Use number of SSA instructions for the Brillig unrolling bytecode size limit (noir-lang/noir#7242)
feat: Sync from aztec-packages (noir-lang/noir#7241)
chore: bump gates diff (noir-lang/noir#7245)
feat: simplify subtraction from self to return zero (noir-lang/noir#7189)
feat: allow specifying multiple patterns in nargo test (noir-lang/noir#7186)
fix: Avoid type error when calling something with a type alias of a function (noir-lang/noir#7239)
feat: Allow resolved types in constructors (noir-lang/noir#7223)
chore: clarify to_radix docs examples (noir-lang/noir#7230)
chore: fix struct example (noir-lang/noir#7198)
feat(optimization): Add purity analysis to SSA (noir-lang/noir#7197)
chore: start tracking time to run critical library tests (noir-lang/noir#7221)
chore: Rework defunctionalize pass to not rely on DFG bugs (noir-lang/noir#7222)
fix(brillig): Globals entry point reachability analysis  (noir-lang/noir#7188)
chore: update docs to use devcontainer feature (noir-lang/noir#7206)
chore(ci): Add test for global vars entry points regression (noir-lang/noir#7209)
chore(ssa): Flip the SSA Brillig constraint check to off by default (noir-lang/noir#7211)
chore(docs): moving references to noir-starter to awesome-noir (noir-lang/noir#7203)
chore: build docs in the merge queue (noir-lang/noir#7218)
fix: correct reversed callstacks (noir-lang/noir#7212)
chore: exclude dependency fetching time from benchmarks (noir-lang/noir#7210)
feat(experimental): Support enums in comptime code (noir-lang/noir#7194)
  • Loading branch information
AztecBot committed Feb 1, 2025
2 parents 55aff0e + 05ed73b commit 70771ab
Show file tree
Hide file tree
Showing 15 changed files with 158 additions and 79 deletions.
2 changes: 1 addition & 1 deletion .noir-sync-commit
Original file line number Diff line number Diff line change
@@ -1 +1 @@
a2a0478eaa1cd372b870e9b1f152877f0dcf9fd1
a9e985064303b0843cbf68fb5a9d41f9ade1e30d
10 changes: 10 additions & 0 deletions noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,16 @@ impl Function {

unreachable!("SSA Function {} has no reachable return instruction!", self.id())
}

pub(crate) fn num_instructions(&self) -> usize {
self.reachable_blocks()
.iter()
.map(|block| {
let block = &self.dfg[*block];
block.instructions().len() + block.terminator().is_some() as usize
})
.sum()
}
}

impl Clone for Function {
Expand Down
40 changes: 2 additions & 38 deletions noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,6 @@ use acvm::{acir::AcirField, FieldElement};
use im::HashSet;

use crate::{
brillig::{
brillig_gen::{brillig_globals::convert_ssa_globals, convert_ssa_function},
brillig_ir::brillig_variable::BrilligVariable,
},
errors::RuntimeError,
ssa::{
ir::{
Expand Down Expand Up @@ -60,8 +56,6 @@ impl Ssa {
mut self,
max_bytecode_increase_percent: Option<i32>,
) -> Result<Ssa, RuntimeError> {
let mut global_cache = None;

for function in self.functions.values_mut() {
let is_brillig = function.runtime().is_brillig();

Expand All @@ -78,20 +72,9 @@ impl Ssa {
// to the globals and a mutable reference to the function at the same time, both part of the `Ssa`.
if has_unrolled && is_brillig {
if let Some(max_incr_pct) = max_bytecode_increase_percent {
if global_cache.is_none() {
let globals = (*function.dfg.globals).clone();
let used_globals = &globals.values_iter().map(|(id, _)| id).collect();
let globals_dfg = DataFlowGraph::from(globals);
// DIE is run at the end of our SSA optimizations, so we mark all globals as in use here.
let (_, brillig_globals, _) =
convert_ssa_globals(false, &globals_dfg, used_globals, function.id());
global_cache = Some(brillig_globals);
}
let brillig_globals = global_cache.as_ref().unwrap();

let orig_function = orig_function.expect("took snapshot to compare");
let new_size = brillig_bytecode_size(function, brillig_globals);
let orig_size = brillig_bytecode_size(&orig_function, brillig_globals);
let new_size = function.num_instructions();
let orig_size = orig_function.num_instructions();
if !is_new_size_ok(orig_size, new_size, max_incr_pct) {
*function = orig_function;
}
Expand Down Expand Up @@ -1022,25 +1005,6 @@ fn simplify_between_unrolls(function: &mut Function) {
function.mem2reg();
}

/// Convert the function to Brillig bytecode and return the resulting size.
fn brillig_bytecode_size(
function: &Function,
globals: &HashMap<ValueId, BrilligVariable>,
) -> usize {
// We need to do some SSA passes in order for the conversion to be able to go ahead,
// otherwise we can hit `unreachable!()` instructions in `convert_ssa_instruction`.
// Creating a clone so as not to modify the originals.
let mut temp = function.clone();

// Might as well give it the best chance.
simplify_between_unrolls(&mut temp);

// This is to try to prevent hitting ICE.
temp.dead_instruction_elimination(false, true);

convert_ssa_function(&temp, false, globals).byte_code.len()
}

/// Decide if the new bytecode size is acceptable, compared to the original.
///
/// The maximum increase can be expressed as a negative value if we demand a decrease.
Expand Down
14 changes: 11 additions & 3 deletions noir/noir-repo/compiler/noirc_frontend/src/ast/enumeration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,11 @@ impl NoirEnumeration {
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct EnumVariant {
pub name: Ident,
pub parameters: Vec<UnresolvedType>,

/// This is None for tag variants without parameters.
/// A value of `Some(vec![])` corresponds to a variant defined as `Foo()`
/// with parenthesis but no parameters.
pub parameters: Option<Vec<UnresolvedType>>,
}

impl Display for NoirEnumeration {
Expand All @@ -41,8 +45,12 @@ impl Display for NoirEnumeration {
writeln!(f, "enum {}{} {{", self.name, generics)?;

for variant in self.variants.iter() {
let parameters = vecmap(&variant.item.parameters, ToString::to_string).join(", ");
writeln!(f, " {}({}),", variant.item.name, parameters)?;
if let Some(parameters) = &variant.item.parameters {
let parameters = vecmap(parameters, ToString::to_string).join(", ");
writeln!(f, " {}({}),", variant.item.name, parameters)?;
} else {
writeln!(f, " {},", variant.item.name)?;
}
}

write!(f, "}}")
Expand Down
6 changes: 4 additions & 2 deletions noir/noir-repo/compiler/noirc_frontend/src/ast/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -795,8 +795,10 @@ impl NoirEnumeration {
}

for variant in &self.variants {
for parameter in &variant.item.parameters {
parameter.accept(visitor);
if let Some(parameters) = &variant.item.parameters {
for parameter in parameters {
parameter.accept(visitor);
}
}
}
}
Expand Down
110 changes: 100 additions & 10 deletions noir/noir-repo/compiler/noirc_frontend/src/elaborator/enums.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,104 @@ use crate::{
function::{FuncMeta, FunctionBody, HirFunction, Parameters},
stmt::HirPattern,
},
node_interner::{DefinitionKind, FuncId, FunctionModifiers, TypeId},
node_interner::{DefinitionKind, ExprId, FunctionModifiers, GlobalValue, TypeId},
token::Attributes,
DataType, Shared, Type,
};

use super::Elaborator;

impl Elaborator<'_> {
/// Defines the value of an enum variant that we resolve an enum
/// variant expression to. E.g. `Foo::Bar` in `Foo::Bar(baz)`.
///
/// If the variant requires arguments we should define a function,
/// otherwise we define a polymorphic global containing the tag value.
#[allow(clippy::too_many_arguments)]
pub(super) fn define_enum_variant_function(
pub(super) fn define_enum_variant_constructor(
&mut self,
enum_: &NoirEnumeration,
type_id: TypeId,
variant: &EnumVariant,
variant_arg_types: Option<Vec<Type>>,
variant_index: usize,
datatype: &Shared<DataType>,
self_type: &Type,
self_type_unresolved: UnresolvedType,
) {
match variant_arg_types {
Some(args) => self.define_enum_variant_function(
enum_,
type_id,
variant,
args,
variant_index,
datatype,
self_type,
self_type_unresolved,
),
None => self.define_enum_variant_global(
enum_,
type_id,
variant,
variant_index,
datatype,
self_type,
),
}
}

#[allow(clippy::too_many_arguments)]
fn define_enum_variant_global(
&mut self,
enum_: &NoirEnumeration,
type_id: TypeId,
variant: &EnumVariant,
variant_index: usize,
datatype: &Shared<DataType>,
self_type: &Type,
) {
let name = &variant.name;
let location = Location::new(variant.name.span(), self.file);

let global_id = self.interner.push_empty_global(
name.clone(),
type_id.local_module_id(),
type_id.krate(),
self.file,
Vec::new(),
false,
false,
);

let mut typ = self_type.clone();
if !datatype.borrow().generics.is_empty() {
let typevars = vecmap(&datatype.borrow().generics, |generic| generic.type_var.clone());
typ = Type::Forall(typevars, Box::new(typ));
}

let definition_id = self.interner.get_global(global_id).definition_id;
self.interner.push_definition_type(definition_id, typ.clone());

let no_parameters = Parameters(Vec::new());
let global_body =
self.make_enum_variant_constructor(datatype, variant_index, &no_parameters, location);
let let_statement = crate::hir_def::stmt::HirStatement::Expression(global_body);

let statement_id = self.interner.get_global(global_id).let_statement;
self.interner.replace_statement(statement_id, let_statement);

self.interner.get_global_mut(global_id).value = GlobalValue::Resolved(
crate::hir::comptime::Value::Enum(variant_index, Vec::new(), typ),
);

Self::get_module_mut(self.def_maps, type_id.module_id())
.declare_global(name.clone(), enum_.visibility, global_id)
.ok();
}

#[allow(clippy::too_many_arguments)]
fn define_enum_variant_function(
&mut self,
enum_: &NoirEnumeration,
type_id: TypeId,
Expand Down Expand Up @@ -48,7 +136,10 @@ impl Elaborator<'_> {

let hir_name = HirIdent::non_trait_method(definition_id, location);
let parameters = self.make_enum_variant_parameters(variant_arg_types, location);
self.push_enum_variant_function_body(id, datatype, variant_index, &parameters, location);

let body =
self.make_enum_variant_constructor(datatype, variant_index, &parameters, location);
self.interner.update_fn(id, HirFunction::unchecked_from_expr(body));

let function_type =
datatype_ref.variant_function_type_with_forall(variant_index, datatype.clone());
Expand Down Expand Up @@ -106,14 +197,13 @@ impl Elaborator<'_> {
// }
// }
// ```
fn push_enum_variant_function_body(
fn make_enum_variant_constructor(
&mut self,
id: FuncId,
self_type: &Shared<DataType>,
variant_index: usize,
parameters: &Parameters,
location: Location,
) {
) -> ExprId {
// Each parameter of the enum variant function is used as a parameter of the enum
// constructor expression
let arguments = vecmap(&parameters.0, |(pattern, typ, _)| match pattern {
Expand All @@ -126,18 +216,18 @@ impl Elaborator<'_> {
_ => unreachable!(),
});

let enum_generics = self_type.borrow().generic_types();
let construct_variant = HirExpression::EnumConstructor(HirEnumConstructorExpression {
let constructor = HirExpression::EnumConstructor(HirEnumConstructorExpression {
r#type: self_type.clone(),
arguments,
variant_index,
});
let body = self.interner.push_expr(construct_variant);
self.interner.update_fn(id, HirFunction::unchecked_from_expr(body));

let body = self.interner.push_expr(constructor);
let enum_generics = self_type.borrow().generic_types();
let typ = Type::DataType(self_type.clone(), enum_generics);
self.interner.push_expr_type(body, typ);
self.interner.push_expr_location(body, location.span, location.file);
body
}

fn make_enum_variant_parameters(
Expand Down
14 changes: 7 additions & 7 deletions noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1841,16 +1841,16 @@ impl<'context> Elaborator<'context> {
let module_id = ModuleId { krate: self.crate_id, local_id: typ.module_id };

for (i, variant) in typ.enum_def.variants.iter().enumerate() {
let types = vecmap(&variant.item.parameters, |typ| self.resolve_type(typ.clone()));
let parameters = variant.item.parameters.as_ref();
let types =
parameters.map(|params| vecmap(params, |typ| self.resolve_type(typ.clone())));
let name = variant.item.name.clone();

// false here is for the eventual change to allow enum "constants" rather than
// always having them be called as functions. This can be replaced with an actual
// check once #7172 is implemented.
datatype.borrow_mut().push_variant(EnumVariant::new(name, types.clone(), false));
let is_function = types.is_some();
let params = types.clone().unwrap_or_default();
datatype.borrow_mut().push_variant(EnumVariant::new(name, params, is_function));

// Define a function for each variant to construct it
self.define_enum_variant_function(
self.define_enum_variant_constructor(
&typ.enum_def,
*type_id,
&variant.item,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1294,7 +1294,7 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {
id: ExprId,
) -> IResult<Value> {
let fields = try_vecmap(constructor.arguments, |arg| self.evaluate(arg))?;
let typ = self.elaborator.interner.id_type(id).follow_bindings();
let typ = self.elaborator.interner.id_type(id).unwrap_forall().1.follow_bindings();
Ok(Value::Enum(constructor.variant_index, fields, typ))
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,8 @@ impl Value {
})
}
Value::Enum(variant_index, args, typ) => {
let r#type = match typ.follow_bindings() {
// Enum constants can have generic types but aren't functions
let r#type = match typ.unwrap_forall().1.follow_bindings() {
Type::DataType(def, _) => def,
_ => return Err(InterpreterError::NonEnumInConstructor { typ, location }),
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ impl CollectedItems {
pub fn is_empty(&self) -> bool {
self.functions.is_empty()
&& self.structs.is_empty()
&& self.enums.is_empty()
&& self.type_aliases.is_empty()
&& self.traits.is_empty()
&& self.globals.is_empty()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2209,7 +2209,7 @@ fn unwrap_enum_type(
typ: &HirType,
location: Location,
) -> Result<Vec<(String, Vec<HirType>)>, MonomorphizationError> {
match typ.follow_bindings() {
match typ.unwrap_forall().1.follow_bindings() {
HirType::DataType(def, args) => {
// Some of args might not be mentioned in fields, so we need to check that they aren't unbound.
for arg in &args {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,12 +92,10 @@ impl<'a> Parser<'a> {
self.bump();
}

let mut parameters = Vec::new();

if self.eat_left_paren() {
let parameters = self.eat_left_paren().then(|| {
let comma_separated = separated_by_comma_until_right_paren();
parameters = self.parse_many("variant parameters", comma_separated, Self::parse_type);
}
self.parse_many("variant parameters", comma_separated, Self::parse_type)
});

Some(Documented::new(EnumVariant { name, parameters }, doc_comments))
}
Expand Down Expand Up @@ -189,18 +187,19 @@ mod tests {
let variant = noir_enum.variants.remove(0).item;
assert_eq!("X", variant.name.to_string());
assert!(matches!(
variant.parameters[0].typ,
variant.parameters.as_ref().unwrap()[0].typ,
UnresolvedTypeData::Integer(Signedness::Signed, IntegerBitSize::ThirtyTwo)
));

let variant = noir_enum.variants.remove(0).item;
assert_eq!("y", variant.name.to_string());
assert!(matches!(variant.parameters[0].typ, UnresolvedTypeData::FieldElement));
assert!(matches!(variant.parameters[1].typ, UnresolvedTypeData::Integer(..)));
let parameters = variant.parameters.as_ref().unwrap();
assert!(matches!(parameters[0].typ, UnresolvedTypeData::FieldElement));
assert!(matches!(parameters[1].typ, UnresolvedTypeData::Integer(..)));

let variant = noir_enum.variants.remove(0).item;
assert_eq!("Z", variant.name.to_string());
assert_eq!(variant.parameters.len(), 0);
assert!(variant.parameters.is_none());
}

#[test]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ fn main() {
comptime {
let _two = Foo::Couple(1, 2);
let _one = Foo::One(3);
let _none = Foo::None();
let _none = Foo::None;
}
}

Expand Down
Loading

0 comments on commit 70771ab

Please sign in to comment.