From f6810af9901f5f598d3ddc42bdbb31d095958622 Mon Sep 17 00:00:00 2001 From: kevaundray Date: Tue, 6 Jun 2023 21:31:54 +0000 Subject: [PATCH 01/14] make ranges be polymorphic integers --- crates/noirc_frontend/src/hir/type_check/expr.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/noirc_frontend/src/hir/type_check/expr.rs b/crates/noirc_frontend/src/hir/type_check/expr.rs index 55a14860a1..cd5c958431 100644 --- a/crates/noirc_frontend/src/hir/type_check/expr.rs +++ b/crates/noirc_frontend/src/hir/type_check/expr.rs @@ -153,9 +153,12 @@ impl<'interner> TypeChecker<'interner> { let start_span = self.interner.expr_span(&for_expr.start_range); let end_span = self.interner.expr_span(&for_expr.end_range); - let mut unify_loop_range = |actual_type, span| { + let mut unify_loop_range = |actual_type: &Type, span: Span| { let expected_type = if self.is_unconstrained() { - Type::FieldElement(CompTime::new(self.interner)) + Type::PolymorphicInteger( + CompTime::new(self.interner), + Shared::new(TypeBinding::Bound(actual_type.clone())), + ) } else { Type::comp_time(Some(span)) }; From 66594a58fddc9276d3d751faaa3ae3abd73c8151 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Thu, 8 Jun 2023 09:38:12 +0000 Subject: [PATCH 02/14] feat: brillig loop support --- .../brillig_loop/Nargo.toml | 5 + .../brillig_loop/Prover.toml | 2 + .../brillig_loop/src/main.nr | 14 ++ crates/noirc_evaluator/src/brillig/binary.rs | 121 ++++++++++++++++ .../src/brillig/brillig_gen.rs | 132 +++++++----------- crates/noirc_evaluator/src/brillig/memory.rs | 15 ++ crates/noirc_evaluator/src/brillig/mod.rs | 2 + crates/noirc_evaluator/src/ssa/ssa_gen.rs | 4 +- .../src/ssa_refactor/ssa_gen/mod.rs | 12 +- .../src/hir/resolution/resolver.rs | 4 +- .../noirc_frontend/src/hir/type_check/expr.rs | 7 +- .../src/monomorphization/mod.rs | 3 +- 12 files changed, 226 insertions(+), 95 deletions(-) create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/brillig_loop/Nargo.toml create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/brillig_loop/Prover.toml create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/brillig_loop/src/main.nr create mode 100644 crates/noirc_evaluator/src/brillig/binary.rs create mode 100644 crates/noirc_evaluator/src/brillig/memory.rs diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_loop/Nargo.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_loop/Nargo.toml new file mode 100644 index 0000000000..e0b467ce5d --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_loop/Nargo.toml @@ -0,0 +1,5 @@ +[package] +authors = [""] +compiler_version = "0.1" + +[dependencies] \ No newline at end of file diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_loop/Prover.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_loop/Prover.toml new file mode 100644 index 0000000000..11b83bc005 --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_loop/Prover.toml @@ -0,0 +1,2 @@ +iterations = "5" +sum = "10" diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_loop/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_loop/src/main.nr new file mode 100644 index 0000000000..475406f810 --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_loop/src/main.nr @@ -0,0 +1,14 @@ +// Tests a very simple program. +// +// The features being tested is basic looping on brillig +fn main(iterations: u32, sum: u32){ + assert(sum == loop(iterations)); +} + +unconstrained fn loop(x: u32) -> u32 { + let mut sum = 0; + for i in 0..x { + sum = sum + i; + } + sum +} diff --git a/crates/noirc_evaluator/src/brillig/binary.rs b/crates/noirc_evaluator/src/brillig/binary.rs new file mode 100644 index 0000000000..f1a18393c1 --- /dev/null +++ b/crates/noirc_evaluator/src/brillig/binary.rs @@ -0,0 +1,121 @@ +use crate::ssa_refactor::ir::{ + instruction::BinaryOp, + types::{NumericType, Type}, +}; +use acvm::acir::brillig_vm::{BinaryFieldOp, BinaryIntOp}; + +/// Type to encapsulate the binary operation types in Brillig +pub(crate) enum BrilligBinaryOp { + Field { op: BinaryFieldOp }, + Integer { op: BinaryIntOp, bit_size: u32 }, +} + +impl BrilligBinaryOp { + /// Convert an SSA binary operation into: + /// - Brillig Binary Integer Op, if it is a integer type + /// - Brillig Binary Field Op, if it is a field type + pub(crate) fn convert_ssa_binary_op_to_brillig_binary_op( + ssa_op: BinaryOp, + typ: Type, + ) -> BrilligBinaryOp { + // First get the bit size and whether its a signed integer, if it is a numeric type + // if it is not,then we return None, indicating that + // it is a Field. + let bit_size_signedness = match typ { + Type::Numeric(numeric_type) => match numeric_type { + NumericType::Signed { bit_size } => Some((bit_size, true)), + NumericType::Unsigned { bit_size } => Some((bit_size, false)), + NumericType::NativeField => None, + }, + _ => unreachable!("only numeric types are allowed in binary operations. References are handled separately"), + }; + + fn binary_op_to_field_op(op: BinaryOp) -> BinaryFieldOp { + match op { + BinaryOp::Add => BinaryFieldOp::Add, + BinaryOp::Sub => BinaryFieldOp::Sub, + BinaryOp::Mul => BinaryFieldOp::Mul, + BinaryOp::Div => BinaryFieldOp::Div, + BinaryOp::Eq => BinaryFieldOp::Equals, + _ => unreachable!( + "Field type cannot be used with {op}. This should have been caught by the frontend" + ), + } + } + fn binary_op_to_int_op(op: BinaryOp, is_signed: bool) -> BinaryIntOp { + match op { + BinaryOp::Add => BinaryIntOp::Add, + BinaryOp::Sub => BinaryIntOp::Sub, + BinaryOp::Mul => BinaryIntOp::Mul, + BinaryOp::Div => { + if is_signed { + BinaryIntOp::SignedDiv + } else { + BinaryIntOp::UnsignedDiv + } + }, + BinaryOp::Mod => todo!("This is not supported by Brillig. It should either be added into Brillig or legalized by the SSA IR"), + BinaryOp::Eq => BinaryIntOp::Equals, + BinaryOp::Lt => BinaryIntOp::LessThan, + BinaryOp::And => BinaryIntOp::And, + BinaryOp::Or => BinaryIntOp::Or, + BinaryOp::Xor => BinaryIntOp::Xor, + BinaryOp::Shl => BinaryIntOp::Shl, + BinaryOp::Shr => BinaryIntOp::Shr, + } + } + // If bit size is available then it is a binary integer operation + match bit_size_signedness { + Some((bit_size, is_signed)) => { + let binary_int_op = binary_op_to_int_op(ssa_op, is_signed); + BrilligBinaryOp::Integer { op: binary_int_op, bit_size } + } + None => { + let binary_field_op = binary_op_to_field_op(ssa_op); + BrilligBinaryOp::Field { op: binary_field_op } + } + } + } +} + +/// Operands in a binary operation are checked to have the same type. +/// +/// In Noir, binary operands should have the same type due to the language +/// semantics. +/// +/// There are some edge cases to consider: +/// - Constants are not explicitly type casted, so we need to check for this and +/// return the type of the other operand, if we have a constant. +/// - 0 is not seen as `Field 0` but instead as `Unit 0` +/// TODO: The latter seems like a bug, if we cannot differentiate between a function returning +/// TODO nothing and a 0. +/// +/// TODO: This constant coercion should ideally be done in the type checker. +pub(crate) fn type_of_binary_operation(lhs_type: Type, rhs_type: Type) -> Type { + match (lhs_type, rhs_type) { + // Function type should not be possible, since all functions + // have been inlined. + (Type::Function, _) | (_, Type::Function) => { + unreachable!("ICE: Function type not allowed in binary operations") + } + (_, Type::Reference) | (Type::Reference, _) => { + unreachable!("ICE: Reference reached a binary operation") + } + // Unit type currently can mean a 0 constant, so we return the + // other type. + (typ, Type::Unit) | (Type::Unit, typ) => typ, + // If either side is a Field constant then, we coerce into the type + // of the other operand + (Type::Numeric(NumericType::NativeField), typ) + | (typ, Type::Numeric(NumericType::NativeField)) => typ, + // If either side is a numeric type, then we expect their types to be + // the same. + (Type::Numeric(lhs_type), Type::Numeric(rhs_type)) => { + assert_eq!( + lhs_type, rhs_type, + "lhs and rhs types in a binary operation are always the same" + ); + Type::Numeric(lhs_type) + } + } +} diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen.rs b/crates/noirc_evaluator/src/brillig/brillig_gen.rs index f57503dd5e..b5bdee5367 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen.rs @@ -1,16 +1,18 @@ -use super::artifact::BrilligArtifact; +use super::{ + artifact::BrilligArtifact, + binary::{type_of_binary_operation, BrilligBinaryOp}, + memory::BrilligMemory, +}; use crate::ssa_refactor::ir::{ basic_block::{BasicBlock, BasicBlockId}, dfg::DataFlowGraph, function::Function, - instruction::{Binary, BinaryOp, Instruction, InstructionId, TerminatorInstruction}, + instruction::{Binary, Instruction, InstructionId, TerminatorInstruction}, post_order::PostOrder, - types::{NumericType, Type}, + types::Type, value::{Value, ValueId}, }; -use acvm::acir::brillig_vm::{ - BinaryFieldOp, BinaryIntOp, Opcode as BrilligOpcode, RegisterIndex, Value as BrilligValue, -}; +use acvm::acir::brillig_vm::{Opcode as BrilligOpcode, RegisterIndex, Value as BrilligValue}; use std::collections::HashMap; #[derive(Default)] @@ -21,6 +23,8 @@ pub(crate) struct BrilligGen { latest_register: usize, /// Map from SSA values to Register Indices. ssa_value_to_register: HashMap, + /// Tracks memory allocations + memory: BrilligMemory, } impl BrilligGen { @@ -48,7 +52,7 @@ impl BrilligGen { /// Converts an SSA Basic block into a sequence of Brillig opcodes fn convert_block(&mut self, block_id: BasicBlockId, dfg: &DataFlowGraph) { self.obj.add_block_label(block_id); - let block = &dfg[dbg!(block_id)]; + let block = &dfg[block_id]; self.convert_block_params(block, dfg); for instruction_id in block.instructions() { @@ -146,10 +150,40 @@ impl BrilligGen { let result_register = self.get_or_create_register(result_ids[0]); self.convert_ssa_binary(binary, dfg, result_register); } + Instruction::Allocate { size } => { + let pointer_register = + self.get_or_create_register(dfg.instruction_results(instruction_id)[0]); + self.allocate_array(pointer_register, *size); + } + Instruction::Store { address, value } => { + let address_register = self.convert_ssa_value(*address, dfg); + let value_register = self.convert_ssa_value(*value, dfg); + self.push_code(BrilligOpcode::Store { + destination_pointer: address_register, + source: value_register, + }); + } + Instruction::Load { address } => { + let target_register = + self.get_or_create_register(dfg.instruction_results(instruction_id)[0]); + let address_register = self.convert_ssa_value(*address, dfg); + self.push_code(BrilligOpcode::Load { + destination: target_register, + source_pointer: address_register, + }); + } _ => todo!("ICE: Instruction not supported"), }; } + fn allocate_array(&mut self, pointer_register: RegisterIndex, size: u32) { + let free_pointer = self.memory.allocate(size as usize); + self.push_code(BrilligOpcode::Const { + destination: pointer_register, + value: BrilligValue::from(free_pointer), + }); + } + /// Converts the Binary instruction into a sequence of Brillig opcodes. fn convert_ssa_binary( &mut self, @@ -157,17 +191,16 @@ impl BrilligGen { dfg: &DataFlowGraph, result_register: RegisterIndex, ) { - let left_type = dfg[binary.lhs].get_type(); - let right_type = dfg[binary.rhs].get_type(); - if left_type != right_type { - todo!("ICE: Binary operands must have the same type") - } + let binary_type = + type_of_binary_operation(dfg[binary.lhs].get_type(), dfg[binary.rhs].get_type()); let left = self.convert_ssa_value(binary.lhs, dfg); let right = self.convert_ssa_value(binary.rhs, dfg); - let brillig_binary_op = - BrilligBinaryOp::convert_ssa_binary_op_to_brillig_binary_op(binary.operator, left_type); + let brillig_binary_op = BrilligBinaryOp::convert_ssa_binary_op_to_brillig_binary_op( + binary.operator, + binary_type, + ); match brillig_binary_op { BrilligBinaryOp::Field { op } => { let opcode = BrilligOpcode::BinaryFieldOp { @@ -241,76 +274,7 @@ impl BrilligGen { for block in reverse_post_order { self.convert_block(block, &func.dfg); } - } -} -/// Type to encapsulate the binary operation types in Brillig -pub(crate) enum BrilligBinaryOp { - Field { op: BinaryFieldOp }, - Integer { op: BinaryIntOp, bit_size: u32 }, -} - -impl BrilligBinaryOp { - /// Convert an SSA binary operation into: - /// - Brillig Binary Integer Op, if it is a integer type - /// - Brillig Binary Field Op, if it is a field type - fn convert_ssa_binary_op_to_brillig_binary_op(ssa_op: BinaryOp, typ: Type) -> BrilligBinaryOp { - // First get the bit size and whether its a signed integer, if it is a numeric type - // if it is not,then we return None, indicating that - // it is a Field. - let bit_size_signedness = match typ { - Type::Numeric(numeric_type) => match numeric_type { - NumericType::Signed { bit_size } => Some((bit_size, true)), - NumericType::Unsigned { bit_size } => Some((bit_size, false)), - NumericType::NativeField => None, - }, - _ => unreachable!("only numeric types are allowed in binary operations. References are handled separately"), - }; - - fn binary_op_to_field_op(op: BinaryOp) -> BinaryFieldOp { - match op { - BinaryOp::Add => BinaryFieldOp::Add, - BinaryOp::Sub => BinaryFieldOp::Sub, - BinaryOp::Mul => BinaryFieldOp::Mul, - BinaryOp::Div => BinaryFieldOp::Div, - BinaryOp::Eq => BinaryFieldOp::Equals, - _ => unreachable!( - "Field type cannot be used with {op}. This should have been caught by the frontend" - ), - } - } - fn binary_op_to_int_op(op: BinaryOp, is_signed: bool) -> BinaryIntOp { - match op { - BinaryOp::Add => BinaryIntOp::Add, - BinaryOp::Sub => BinaryIntOp::Sub, - BinaryOp::Mul => BinaryIntOp::Mul, - BinaryOp::Div => { - if is_signed { - BinaryIntOp::SignedDiv - } else { - BinaryIntOp::UnsignedDiv - } - }, - BinaryOp::Mod => todo!("This is not supported by Brillig. It should either be added into Brillig or legalized by the SSA IR"), - BinaryOp::Eq => BinaryIntOp::Equals, - BinaryOp::Lt => BinaryIntOp::LessThan, - BinaryOp::And => BinaryIntOp::And, - BinaryOp::Or => BinaryIntOp::Or, - BinaryOp::Xor => BinaryIntOp::Xor, - BinaryOp::Shl => BinaryIntOp::Shl, - BinaryOp::Shr => BinaryIntOp::Shr, - } - } - // If bit size is available then it is a binary integer operation - match bit_size_signedness { - Some((bit_size, is_signed)) => { - let binary_int_op = binary_op_to_int_op(ssa_op, is_signed); - BrilligBinaryOp::Integer { op: binary_int_op, bit_size } - } - None => { - let binary_field_op = binary_op_to_field_op(ssa_op); - BrilligBinaryOp::Field { op: binary_field_op } - } - } + println!("Brillig gen done"); } } diff --git a/crates/noirc_evaluator/src/brillig/memory.rs b/crates/noirc_evaluator/src/brillig/memory.rs new file mode 100644 index 0000000000..099dbfb770 --- /dev/null +++ b/crates/noirc_evaluator/src/brillig/memory.rs @@ -0,0 +1,15 @@ +/// Simple memory allocator for brillig +/// For now it just tracks a free memory pointer +/// Will probably get smarter in the future +#[derive(Default)] +pub(crate) struct BrilligMemory { + free_mem_pointer: usize, +} + +impl BrilligMemory { + pub(crate) fn allocate(&mut self, size: usize) -> usize { + let start = self.free_mem_pointer; + self.free_mem_pointer += size; + start + } +} diff --git a/crates/noirc_evaluator/src/brillig/mod.rs b/crates/noirc_evaluator/src/brillig/mod.rs index 4c62554615..5c28619194 100644 --- a/crates/noirc_evaluator/src/brillig/mod.rs +++ b/crates/noirc_evaluator/src/brillig/mod.rs @@ -3,7 +3,9 @@ use std::collections::HashMap; use self::{artifact::BrilligArtifact, brillig_gen::BrilligGen}; pub(crate) mod artifact; +pub(crate) mod binary; pub(crate) mod brillig_gen; +pub(crate) mod memory; use crate::ssa_refactor::{ ir::function::{Function, FunctionId, RuntimeType}, diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen.rs b/crates/noirc_evaluator/src/ssa/ssa_gen.rs index 57187a9fcc..31817b0bde 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen.rs @@ -189,7 +189,9 @@ impl IrGenerator { let function_node_id = self.context.get_or_create_opcode_node_id(opcode); Ok(Value::Node(function_node_id)) } - Definition::Oracle(_, _) => unimplemented!("oracles not supported by deprecated SSA") + Definition::Oracle(_, _) => { + unimplemented!("oracles not supported by deprecated SSA") + } } } } diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs index e46fa63200..b7d9dcee87 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs @@ -89,12 +89,12 @@ impl<'a> FunctionContext<'a> { match &ident.definition { ast::Definition::Local(id) => self.lookup(*id).map(|value| value.eval(self).into()), ast::Definition::Function(id) => self.get_or_queue_function(*id), - ast::Definition::Builtin(name) | ast::Definition::LowLevel(name) | ast::Definition::Oracle(name, _) => { - match self.builder.import_intrinsic(name) { - Some(builtin) => builtin.into(), - None => panic!("No builtin function named '{name}' found"), - } - } + ast::Definition::Builtin(name) + | ast::Definition::LowLevel(name) + | ast::Definition::Oracle(name, _) => match self.builder.import_intrinsic(name) { + Some(builtin) => builtin.into(), + None => panic!("No builtin function named '{name}' found"), + }, } } diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index dbdfda985d..67d0e2cf2a 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -307,7 +307,9 @@ impl<'a> Resolver<'a> { let func_meta = self.extract_meta(&func, id); let hir_func = match func.kind { - FunctionKind::Builtin | FunctionKind::LowLevel | FunctionKind::Oracle => HirFunction::empty(), + FunctionKind::Builtin | FunctionKind::LowLevel | FunctionKind::Oracle => { + HirFunction::empty() + } FunctionKind::Normal => { let expr_id = self.intern_block(func.def.body); self.interner.push_expr_location(expr_id, func.def.span, self.file); diff --git a/crates/noirc_frontend/src/hir/type_check/expr.rs b/crates/noirc_frontend/src/hir/type_check/expr.rs index 55a14860a1..cd5c958431 100644 --- a/crates/noirc_frontend/src/hir/type_check/expr.rs +++ b/crates/noirc_frontend/src/hir/type_check/expr.rs @@ -153,9 +153,12 @@ impl<'interner> TypeChecker<'interner> { let start_span = self.interner.expr_span(&for_expr.start_range); let end_span = self.interner.expr_span(&for_expr.end_range); - let mut unify_loop_range = |actual_type, span| { + let mut unify_loop_range = |actual_type: &Type, span: Span| { let expected_type = if self.is_unconstrained() { - Type::FieldElement(CompTime::new(self.interner)) + Type::PolymorphicInteger( + CompTime::new(self.interner), + Shared::new(TypeBinding::Bound(actual_type.clone())), + ) } else { Type::comp_time(Some(span)) }; diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index 2c44898499..0f8fb0ea05 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -21,7 +21,8 @@ use crate::{ stmt::{HirAssignStatement, HirLValue, HirLetStatement, HirPattern, HirStatement}, }, node_interner::{self, DefinitionKind, NodeInterner, StmtId}, - CompTime, FunctionKind, TypeBinding, TypeBindings, token::Attribute, + token::Attribute, + CompTime, FunctionKind, TypeBinding, TypeBindings, }; use self::ast::{Definition, FuncId, Function, LocalId, Program}; From b5961aeaed35b47fdcc02d664c952f2dedbe5c24 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Thu, 8 Jun 2023 13:20:19 +0000 Subject: [PATCH 03/14] fix: fixed brillig returns and stop --- .../brillig_loop/Prover.toml | 3 +-- .../brillig_loop/src/main.nr | 4 ++-- .../noirc_evaluator/src/brillig/brillig_gen.rs | 17 +++-------------- .../src/ssa_refactor/opt/flatten_cfg.rs | 9 +++++++-- 4 files changed, 13 insertions(+), 20 deletions(-) diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_loop/Prover.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_loop/Prover.toml index 11b83bc005..22cd5b7c12 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_loop/Prover.toml +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_loop/Prover.toml @@ -1,2 +1 @@ -iterations = "5" -sum = "10" +sum = "6" diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_loop/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_loop/src/main.nr index 475406f810..7240264b2a 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_loop/src/main.nr +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_loop/src/main.nr @@ -1,8 +1,8 @@ // Tests a very simple program. // // The features being tested is basic looping on brillig -fn main(iterations: u32, sum: u32){ - assert(sum == loop(iterations)); +fn main(sum: u32){ + assert(loop(4) == sum); } unconstrained fn loop(x: u32) -> u32 { diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen.rs b/crates/noirc_evaluator/src/brillig/brillig_gen.rs index b5bdee5367..b611b0d6cc 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen.rs @@ -101,14 +101,6 @@ impl BrilligGen { /// the Register starting at register index 0. `N` indicates the number of /// return values expected. fn convert_ssa_return(&mut self, return_values: &[ValueId], dfg: &DataFlowGraph) { - // Check if the program returns the `Unit/None` type. - // This type signifies that the program returns nothing. - let is_return_unit_type = - return_values.len() == 1 && dfg.type_of_value(return_values[0]) == Type::Unit; - if is_return_unit_type { - return; - } - for (destination_index, value_id) in return_values.iter().enumerate() { let return_register = self.convert_ssa_value(*value_id, dfg); if destination_index > self.latest_register { @@ -119,6 +111,7 @@ impl BrilligGen { source: return_register, }); } + self.push_code(BrilligOpcode::Stop); } /// Converts SSA Block parameters into Brillig Registers. @@ -177,10 +170,10 @@ impl BrilligGen { } fn allocate_array(&mut self, pointer_register: RegisterIndex, size: u32) { - let free_pointer = self.memory.allocate(size as usize); + let array_pointer = self.memory.allocate(size as usize); self.push_code(BrilligOpcode::Const { destination: pointer_register, - value: BrilligValue::from(free_pointer), + value: BrilligValue::from(array_pointer), }); } @@ -256,8 +249,6 @@ impl BrilligGen { brillig.convert_ssa_function(func); - brillig.push_code(BrilligOpcode::Stop); - brillig.obj } @@ -274,7 +265,5 @@ impl BrilligGen { for block in reverse_post_order { self.convert_block(block, &func.dfg); } - - println!("Brillig gen done"); } } diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs index 05ff22808f..b5e4c1eb72 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs @@ -122,7 +122,7 @@ use crate::ssa_refactor::{ cfg::ControlFlowGraph, dfg::InsertInstructionResult, dom::DominatorTree, - function::Function, + function::{Function, RuntimeType}, instruction::{BinaryOp, Instruction, InstructionId, TerminatorInstruction}, post_order::PostOrder, types::Type, @@ -140,7 +140,12 @@ impl Ssa { /// For more information, see the module-level comment at the top of this file. pub(crate) fn flatten_cfg(mut self) -> Ssa { for function in self.functions.values_mut() { - flatten_function_cfg(function); + match function.runtime() { + RuntimeType::Acir => { + flatten_function_cfg(function); + } + RuntimeType::Brillig => {} + } } self } From 172300c7b1b010292d558c462853a68ffd275fea Mon Sep 17 00:00:00 2001 From: sirasistant Date: Thu, 8 Jun 2023 14:13:46 +0000 Subject: [PATCH 04/14] fix: do not apply constants folding to brillig fns --- Cargo.lock | 15 +++++---------- Cargo.toml | 3 +++ .../src/ssa_refactor/opt/constant_folding.rs | 9 +++++++-- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 20bb8a7ca0..958c9440d1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5,8 +5,7 @@ version = 3 [[package]] name = "acir" version = "0.14.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cfcc9e8065170b7d47cf6718e7788c65c01609a86127eacd885a952b16f46b72" +source = "git+https://github.com/noir-lang/acvm?rev=24ad48ecf149d3c04a325166392bc5910f4d3f1e#24ad48ecf149d3c04a325166392bc5910f4d3f1e" dependencies = [ "acir_field", "brillig_vm", @@ -19,8 +18,7 @@ dependencies = [ [[package]] name = "acir_field" version = "0.14.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3a43b59e8419f485a2a15ababa828e27fd20c00ba2ce90e78efe165df7d47ac0" +source = "git+https://github.com/noir-lang/acvm?rev=24ad48ecf149d3c04a325166392bc5910f4d3f1e#24ad48ecf149d3c04a325166392bc5910f4d3f1e" dependencies = [ "ark-bn254", "ark-ff", @@ -33,8 +31,7 @@ dependencies = [ [[package]] name = "acvm" version = "0.14.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ce0a3b770cf70b29fd41313bb0baf2a920475fd782bd7a37822f5ee1406b2231" +source = "git+https://github.com/noir-lang/acvm?rev=24ad48ecf149d3c04a325166392bc5910f4d3f1e#24ad48ecf149d3c04a325166392bc5910f4d3f1e" dependencies = [ "acir", "acvm_stdlib", @@ -72,8 +69,7 @@ dependencies = [ [[package]] name = "acvm_stdlib" version = "0.14.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9c9adc16581262b8571551d029a48118c05b5d02371eb2d10af6a633bb670aec" +source = "git+https://github.com/noir-lang/acvm?rev=24ad48ecf149d3c04a325166392bc5910f4d3f1e#24ad48ecf149d3c04a325166392bc5910f4d3f1e" dependencies = [ "acir", ] @@ -506,8 +502,7 @@ dependencies = [ [[package]] name = "brillig_vm" version = "0.14.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3f81af1fa12f95a86b2c1a1c15aadc186265853698e3098f5ce9966d1aa64cca" +source = "git+https://github.com/noir-lang/acvm?rev=24ad48ecf149d3c04a325166392bc5910f4d3f1e#24ad48ecf149d3c04a325166392bc5910f4d3f1e" dependencies = [ "acir_field", "num-bigint", diff --git a/Cargo.toml b/Cargo.toml index 8459010651..2fbd25c5ad 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -53,3 +53,6 @@ tower = "0.4" url = "2.2.0" wasm-bindgen = { version = "0.2.83", features = ["serde-serialize"] } wasm-bindgen-test = "0.3.33" + +[patch.crates-io] +acvm = { git = "https://github.com/noir-lang/acvm", rev = "24ad48ecf149d3c04a325166392bc5910f4d3f1e" } \ No newline at end of file diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs index ffbba71915..f9d053a6c2 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs @@ -4,7 +4,7 @@ use iter_extended::vecmap; use crate::ssa_refactor::{ ir::{ - basic_block::BasicBlockId, dfg::InsertInstructionResult, function::Function, + basic_block::BasicBlockId, dfg::InsertInstructionResult, function::{Function, RuntimeType}, instruction::InstructionId, value::ValueId, }, ssa_gen::Ssa, @@ -19,7 +19,12 @@ impl Ssa { /// now be simplified. pub(crate) fn fold_constants(mut self) -> Ssa { for function in self.functions.values_mut() { - constant_fold(function); + match function.runtime() { + RuntimeType::Acir => { + constant_fold(function); + } + RuntimeType::Brillig => {} + } } self } From 743ac3361ee9c527fa4fc307e87b4564dad44799 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Thu, 8 Jun 2023 14:36:28 +0000 Subject: [PATCH 05/14] chore: update acvm pointer, cleanup --- Cargo.lock | 25 +++++++------ Cargo.toml | 7 ++-- crates/noirc_evaluator/src/brillig/binary.rs | 35 ++++++------------- .../src/ssa_refactor/opt/constant_folding.rs | 8 +++-- .../src/ssa_refactor/opt/flatten_cfg.rs | 1 + 5 files changed, 34 insertions(+), 42 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 958c9440d1..d87a23d5cb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4,8 +4,9 @@ version = 3 [[package]] name = "acir" -version = "0.14.1" -source = "git+https://github.com/noir-lang/acvm?rev=24ad48ecf149d3c04a325166392bc5910f4d3f1e#24ad48ecf149d3c04a325166392bc5910f4d3f1e" +version = "0.14.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cafbc2adec7783509c6e06831ef57e672fc89c963c855a5fe0449c1ebb2822bc" dependencies = [ "acir_field", "brillig_vm", @@ -17,8 +18,9 @@ dependencies = [ [[package]] name = "acir_field" -version = "0.14.1" -source = "git+https://github.com/noir-lang/acvm?rev=24ad48ecf149d3c04a325166392bc5910f4d3f1e#24ad48ecf149d3c04a325166392bc5910f4d3f1e" +version = "0.14.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6a33855c911b77f0eddd9e0a6b9410238a5ba80a4956dcf335dd65dd4d2303d6" dependencies = [ "ark-bn254", "ark-ff", @@ -30,8 +32,9 @@ dependencies = [ [[package]] name = "acvm" -version = "0.14.1" -source = "git+https://github.com/noir-lang/acvm?rev=24ad48ecf149d3c04a325166392bc5910f4d3f1e#24ad48ecf149d3c04a325166392bc5910f4d3f1e" +version = "0.14.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0ad7d5020f3cee249e162504deeeb1a83fa2a727e4c6a5277136bdecbe82cf2b" dependencies = [ "acir", "acvm_stdlib", @@ -68,8 +71,9 @@ dependencies = [ [[package]] name = "acvm_stdlib" -version = "0.14.1" -source = "git+https://github.com/noir-lang/acvm?rev=24ad48ecf149d3c04a325166392bc5910f4d3f1e#24ad48ecf149d3c04a325166392bc5910f4d3f1e" +version = "0.14.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4c9bf39b1e3b486f090ca1c0ef1e7d47fabfdb522af58117d1d86de048ac0571" dependencies = [ "acir", ] @@ -501,8 +505,9 @@ dependencies = [ [[package]] name = "brillig_vm" -version = "0.14.1" -source = "git+https://github.com/noir-lang/acvm?rev=24ad48ecf149d3c04a325166392bc5910f4d3f1e#24ad48ecf149d3c04a325166392bc5910f4d3f1e" +version = "0.14.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a577e930e991623dd1ec5f9540b20eb601d06ae9d7f66181b81ff699441fc159" dependencies = [ "acir_field", "num-bigint", diff --git a/Cargo.toml b/Cargo.toml index 2fbd25c5ad..4a0b391fcc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -24,7 +24,7 @@ edition = "2021" rust-version = "1.66" [workspace.dependencies] -acvm = "=0.14.1" +acvm = "=0.14.2" arena = { path = "crates/arena" } fm = { path = "crates/fm" } iter-extended = { path = "crates/iter-extended" } @@ -52,7 +52,4 @@ toml = "0.7.2" tower = "0.4" url = "2.2.0" wasm-bindgen = { version = "0.2.83", features = ["serde-serialize"] } -wasm-bindgen-test = "0.3.33" - -[patch.crates-io] -acvm = { git = "https://github.com/noir-lang/acvm", rev = "24ad48ecf149d3c04a325166392bc5910f4d3f1e" } \ No newline at end of file +wasm-bindgen-test = "0.3.33" \ No newline at end of file diff --git a/crates/noirc_evaluator/src/brillig/binary.rs b/crates/noirc_evaluator/src/brillig/binary.rs index f1a18393c1..70dcc582fb 100644 --- a/crates/noirc_evaluator/src/brillig/binary.rs +++ b/crates/noirc_evaluator/src/brillig/binary.rs @@ -78,37 +78,16 @@ impl BrilligBinaryOp { } } -/// Operands in a binary operation are checked to have the same type. -/// -/// In Noir, binary operands should have the same type due to the language -/// semantics. -/// -/// There are some edge cases to consider: -/// - Constants are not explicitly type casted, so we need to check for this and -/// return the type of the other operand, if we have a constant. -/// - 0 is not seen as `Field 0` but instead as `Unit 0` -/// TODO: The latter seems like a bug, if we cannot differentiate between a function returning -/// TODO nothing and a 0. -/// -/// TODO: This constant coercion should ideally be done in the type checker. +/// Returns the type of the operation considering the types of the operands +/// TODO: SSA issues binary operations between fields and integers. +/// This probably should be explicitely casted in SSA to avoid having to coerce at this level. pub(crate) fn type_of_binary_operation(lhs_type: Type, rhs_type: Type) -> Type { match (lhs_type, rhs_type) { - // Function type should not be possible, since all functions - // have been inlined. - (Type::Function, _) | (_, Type::Function) => { - unreachable!("ICE: Function type not allowed in binary operations") - } - (_, Type::Reference) | (Type::Reference, _) => { - unreachable!("ICE: Reference reached a binary operation") - } - // Unit type currently can mean a 0 constant, so we return the - // other type. - (typ, Type::Unit) | (Type::Unit, typ) => typ, // If either side is a Field constant then, we coerce into the type // of the other operand (Type::Numeric(NumericType::NativeField), typ) | (typ, Type::Numeric(NumericType::NativeField)) => typ, - // If either side is a numeric type, then we expect their types to be + // If both sides are numeric type, then we expect their types to be // the same. (Type::Numeric(lhs_type), Type::Numeric(rhs_type)) => { assert_eq!( @@ -117,5 +96,11 @@ pub(crate) fn type_of_binary_operation(lhs_type: Type, rhs_type: Type) -> Type { ); Type::Numeric(lhs_type) } + _ => { + unreachable!( + "ICE: Binary operation between types {:?} and {:?} is not allowed", + lhs_type, rhs_type + ) + } } } diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs index f9d053a6c2..1dfae790ee 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs @@ -4,8 +4,11 @@ use iter_extended::vecmap; use crate::ssa_refactor::{ ir::{ - basic_block::BasicBlockId, dfg::InsertInstructionResult, function::{Function, RuntimeType}, - instruction::InstructionId, value::ValueId, + basic_block::BasicBlockId, + dfg::InsertInstructionResult, + function::{Function, RuntimeType}, + instruction::InstructionId, + value::ValueId, }, ssa_gen::Ssa, }; @@ -23,6 +26,7 @@ impl Ssa { RuntimeType::Acir => { constant_fold(function); } + // Brillig is already generated at this step, so we cannot operate on brillig functions anymore. RuntimeType::Brillig => {} } } diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs index b5e4c1eb72..354b876367 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs @@ -144,6 +144,7 @@ impl Ssa { RuntimeType::Acir => { flatten_function_cfg(function); } + // Brillig is already generated at this step, so we cannot operate on brillig functions anymore. RuntimeType::Brillig => {} } } From f1eb67a2bb11f2003c06fed1b93f712896ce8b1e Mon Sep 17 00:00:00 2001 From: sirasistant Date: Thu, 8 Jun 2023 14:37:55 +0000 Subject: [PATCH 06/14] style: newline on cargo toml --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 4a0b391fcc..38231e4bb2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -52,4 +52,4 @@ toml = "0.7.2" tower = "0.4" url = "2.2.0" wasm-bindgen = { version = "0.2.83", features = ["serde-serialize"] } -wasm-bindgen-test = "0.3.33" \ No newline at end of file +wasm-bindgen-test = "0.3.33" From 1d52f5e55d74fb17430054464b5adf58bcb2bc7b Mon Sep 17 00:00:00 2001 From: kevaundray Date: Thu, 8 Jun 2023 19:01:40 +0000 Subject: [PATCH 07/14] make behavior consistent --- .../noirc_frontend/src/hir/type_check/expr.rs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/crates/noirc_frontend/src/hir/type_check/expr.rs b/crates/noirc_frontend/src/hir/type_check/expr.rs index cd5c958431..e332d4994d 100644 --- a/crates/noirc_frontend/src/hir/type_check/expr.rs +++ b/crates/noirc_frontend/src/hir/type_check/expr.rs @@ -160,12 +160,15 @@ impl<'interner> TypeChecker<'interner> { Shared::new(TypeBinding::Bound(actual_type.clone())), ) } else { - Type::comp_time(Some(span)) + Type::PolymorphicInteger( + CompTime::Yes(Some(span)), + Shared::new(TypeBinding::Bound(actual_type.clone())), + ) }; self.unify(actual_type, &expected_type, span, || { TypeCheckError::TypeCannotBeUsed { - typ: start_range_type.clone(), + typ: actual_type.clone(), place: "for loop", span, } @@ -176,6 +179,16 @@ impl<'interner> TypeChecker<'interner> { unify_loop_range(&start_range_type, start_span); unify_loop_range(&end_range_type, end_span); + // Check that start range and end range have the same types + let range_span = start_span.merge(end_span); + self.unify(&start_range_type, &end_range_type, range_span, || { + TypeCheckError::TypeMismatch { + expected_typ: start_range_type.to_string(), + expr_typ: end_range_type.to_string(), + expr_span: range_span, + } + }); + self.interner.push_definition_type(for_expr.identifier.id, start_range_type); self.check_expression(&for_expr.block); From 2d3ab61409c7a42edb4adc75477134cb4c9957bb Mon Sep 17 00:00:00 2001 From: kevaundray Date: Thu, 8 Jun 2023 20:19:30 +0000 Subject: [PATCH 08/14] remove closure --- .../noirc_frontend/src/hir/type_check/expr.rs | 47 +++++++++---------- 1 file changed, 21 insertions(+), 26 deletions(-) diff --git a/crates/noirc_frontend/src/hir/type_check/expr.rs b/crates/noirc_frontend/src/hir/type_check/expr.rs index e332d4994d..960e033f88 100644 --- a/crates/noirc_frontend/src/hir/type_check/expr.rs +++ b/crates/noirc_frontend/src/hir/type_check/expr.rs @@ -153,32 +153,6 @@ impl<'interner> TypeChecker<'interner> { let start_span = self.interner.expr_span(&for_expr.start_range); let end_span = self.interner.expr_span(&for_expr.end_range); - let mut unify_loop_range = |actual_type: &Type, span: Span| { - let expected_type = if self.is_unconstrained() { - Type::PolymorphicInteger( - CompTime::new(self.interner), - Shared::new(TypeBinding::Bound(actual_type.clone())), - ) - } else { - Type::PolymorphicInteger( - CompTime::Yes(Some(span)), - Shared::new(TypeBinding::Bound(actual_type.clone())), - ) - }; - - self.unify(actual_type, &expected_type, span, || { - TypeCheckError::TypeCannotBeUsed { - typ: actual_type.clone(), - place: "for loop", - span, - } - .add_context("The range of a loop must be known at compile-time") - }); - }; - - unify_loop_range(&start_range_type, start_span); - unify_loop_range(&end_range_type, end_span); - // Check that start range and end range have the same types let range_span = start_span.merge(end_span); self.unify(&start_range_type, &end_range_type, range_span, || { @@ -189,6 +163,27 @@ impl<'interner> TypeChecker<'interner> { } }); + let expected_type = if self.is_unconstrained() { + Type::PolymorphicInteger( + CompTime::new(self.interner), + Shared::new(TypeBinding::Bound(start_range_type.clone())), + ) + } else { + Type::PolymorphicInteger( + CompTime::Yes(Some(range_span)), + Shared::new(TypeBinding::Bound(start_range_type.clone())), + ) + }; + + self.unify(&start_range_type, &expected_type, range_span, || { + TypeCheckError::TypeCannotBeUsed { + typ: start_range_type.clone(), + place: "for loop", + span: range_span, + } + .add_context("The range of a loop must be known at compile-time") + }); + self.interner.push_definition_type(for_expr.identifier.id, start_range_type); self.check_expression(&for_expr.block); From 801a73912e4deeae7e075ef3636bbabcca79cb3c Mon Sep 17 00:00:00 2001 From: kevaundray Date: Thu, 8 Jun 2023 20:32:40 +0000 Subject: [PATCH 09/14] change index_type --- crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs index cdf8a0e710..a1b262b71c 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs @@ -243,7 +243,8 @@ impl<'a> FunctionContext<'a> { let loop_end = self.builder.insert_block(); // this is the 'i' in `for i in start .. end { block }` - let loop_index = self.builder.add_block_parameter(loop_entry, Type::field()); + let index_type = Self::convert_non_tuple_type(&for_expr.index_type); + let loop_index = self.builder.add_block_parameter(loop_entry, index_type); let start_index = self.codegen_non_tuple_expression(&for_expr.start_range); let end_index = self.codegen_non_tuple_expression(&for_expr.end_range); From 2bcfc2461bdc68bcfd75694496a1451713cf1497 Mon Sep 17 00:00:00 2001 From: jfecher Date: Thu, 8 Jun 2023 15:40:12 -0500 Subject: [PATCH 10/14] Update crates/noirc_frontend/src/hir/type_check/expr.rs --- crates/noirc_frontend/src/hir/type_check/expr.rs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/crates/noirc_frontend/src/hir/type_check/expr.rs b/crates/noirc_frontend/src/hir/type_check/expr.rs index 960e033f88..0c636f11fd 100644 --- a/crates/noirc_frontend/src/hir/type_check/expr.rs +++ b/crates/noirc_frontend/src/hir/type_check/expr.rs @@ -163,17 +163,14 @@ impl<'interner> TypeChecker<'interner> { } }); - let expected_type = if self.is_unconstrained() { - Type::PolymorphicInteger( - CompTime::new(self.interner), - Shared::new(TypeBinding::Bound(start_range_type.clone())), - ) + let expected_comptime = if self.is_unconstrained() { + CompTime::new(self.interner) } else { - Type::PolymorphicInteger( - CompTime::Yes(Some(range_span)), - Shared::new(TypeBinding::Bound(start_range_type.clone())), - ) + CompTime::Yes(Some(range_span)) }; + let fresh_id = self.interner.next_type_variable_id(); + let type_variable = Shared::new(TypeBinding::Unbound(fresh_id)); + let expected_type = Type::PolymorphicInteger(expected_comptime, type_variable); self.unify(&start_range_type, &expected_type, range_span, || { TypeCheckError::TypeCannotBeUsed { From 467576c663cd665d13040d8288d36c7b170bda5d Mon Sep 17 00:00:00 2001 From: kevaundray Date: Thu, 8 Jun 2023 21:09:07 +0000 Subject: [PATCH 11/14] better debug information for unsupported instruction --- crates/noirc_evaluator/src/brillig/brillig_gen.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen.rs b/crates/noirc_evaluator/src/brillig/brillig_gen.rs index 82cd501e26..7a354a7068 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen.rs @@ -205,7 +205,7 @@ impl BrilligGen { }; self.push_code(opcode); } - _ => todo!("ICE: Instruction not supported"), + _ => todo!("ICE: Instruction not supported {instruction:?}"), }; } From 944b619b41a18b5d2b571bffc047b8ca5e8aeefd Mon Sep 17 00:00:00 2001 From: kevaundray Date: Thu, 8 Jun 2023 21:12:43 +0000 Subject: [PATCH 12/14] remove edge case for optimizations --- .../src/ssa_refactor/opt/constant_folding.rs | 8 +------- .../noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs | 8 +------- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs index 612f866ee7..a13bdaa1f3 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs @@ -22,13 +22,7 @@ impl Ssa { /// now be simplified. pub(crate) fn fold_constants(mut self) -> Ssa { for function in self.functions.values_mut() { - match function.runtime() { - RuntimeType::Acir => { - constant_fold(function); - } - // Brillig is already generated at this step, so we cannot operate on brillig functions anymore. - RuntimeType::Brillig => {} - } + constant_fold(function); } self } diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs index 354b876367..0cb605ba61 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs @@ -140,13 +140,7 @@ impl Ssa { /// For more information, see the module-level comment at the top of this file. pub(crate) fn flatten_cfg(mut self) -> Ssa { for function in self.functions.values_mut() { - match function.runtime() { - RuntimeType::Acir => { - flatten_function_cfg(function); - } - // Brillig is already generated at this step, so we cannot operate on brillig functions anymore. - RuntimeType::Brillig => {} - } + flatten_function_cfg(function); } self } From bd248b2300e921a080dfa2211dd2feb087518d18 Mon Sep 17 00:00:00 2001 From: kevaundray Date: Thu, 8 Jun 2023 21:26:43 +0000 Subject: [PATCH 13/14] clippy fix --- .../src/ssa_refactor/opt/constant_folding.rs | 7 ++----- crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs index a13bdaa1f3..5a36ab799c 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs @@ -4,11 +4,8 @@ use iter_extended::vecmap; use crate::ssa_refactor::{ ir::{ - basic_block::BasicBlockId, - dfg::InsertInstructionResult, - function::{Function, RuntimeType}, - instruction::InstructionId, - value::ValueId, + basic_block::BasicBlockId, dfg::InsertInstructionResult, function::Function, + instruction::InstructionId, value::ValueId, }, ssa_gen::Ssa, }; diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs index 0cb605ba61..05ff22808f 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs @@ -122,7 +122,7 @@ use crate::ssa_refactor::{ cfg::ControlFlowGraph, dfg::InsertInstructionResult, dom::DominatorTree, - function::{Function, RuntimeType}, + function::Function, instruction::{BinaryOp, Instruction, InstructionId, TerminatorInstruction}, post_order::PostOrder, types::Type, From 931b25b099f9af709578ea05993b476c93ebe274 Mon Sep 17 00:00:00 2001 From: kevaundray Date: Thu, 8 Jun 2023 22:30:29 +0000 Subject: [PATCH 14/14] patch infinite loop --- .../noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs index 05ff22808f..c0be297e5d 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs @@ -184,6 +184,15 @@ struct Branch { } fn flatten_function_cfg(function: &mut Function) { + // TODO This pass will run forever on a brillig function. + // TODO In particular, analyze will check if the predecessors + // TODO have been processed and push the block to the back of the queue + // TODO This loops forever, if the predecessors are not then processed + // TODO Because it will visit the same block again, pop it out of the queue + // TODO then back into the queue again. + if let crate::ssa_refactor::ir::function::RuntimeType::Brillig = function.runtime() { + return; + } let mut context = Context { cfg: ControlFlowGraph::with_function(function), function,