From c100d34491992606a354f53a9b960ac3223d21c9 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Sat, 11 Feb 2023 16:30:43 +0000 Subject: [PATCH 1/5] - indent NumericType into ObjectType - Create helper methods for common operations involving NumericType --- crates/noirc_evaluator/src/lib.rs | 2 +- .../src/ssa/acir_gen/internal_var_cache.rs | 5 +- .../src/ssa/acir_gen/operations/binary.rs | 2 +- crates/noirc_evaluator/src/ssa/builtin.rs | 16 ++-- crates/noirc_evaluator/src/ssa/code_gen.rs | 11 +-- crates/noirc_evaluator/src/ssa/context.rs | 39 +++++---- crates/noirc_evaluator/src/ssa/integer.rs | 12 +-- crates/noirc_evaluator/src/ssa/node.rs | 81 ++++++++++++++----- .../noirc_evaluator/src/ssa/optimizations.rs | 2 +- 9 files changed, 109 insertions(+), 61 deletions(-) diff --git a/crates/noirc_evaluator/src/lib.rs b/crates/noirc_evaluator/src/lib.rs index a7c5f43fead..16dbba2f366 100644 --- a/crates/noirc_evaluator/src/lib.rs +++ b/crates/noirc_evaluator/src/lib.rs @@ -151,7 +151,7 @@ impl Evaluator { ir_gen.create_new_variable( name.to_owned(), Some(def), - node::ObjectType::NativeField, + node::ObjectType::native_field(), Some(witness), ); } diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/internal_var_cache.rs b/crates/noirc_evaluator/src/ssa/acir_gen/internal_var_cache.rs index 82c46b0054c..08d6d1a6829 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/internal_var_cache.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/internal_var_cache.rs @@ -37,10 +37,7 @@ impl InternalVarCache { NodeObject::Obj(variable) => { let variable_type = variable.get_type(); match variable_type { - ObjectType::Boolean - | ObjectType::NativeField - | ObjectType::Signed(..) - | ObjectType::Unsigned(..) => { + ObjectType::Boolean | ObjectType::Numeric(..) => { let witness = variable.witness.unwrap_or_else(|| evaluator.add_witness_to_cs()); InternalVar::from_witness(witness) diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/operations/binary.rs b/crates/noirc_evaluator/src/ssa/acir_gen/operations/binary.rs index 57bc0782d16..a96d645b9d1 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/operations/binary.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/operations/binary.rs @@ -54,7 +54,7 @@ pub(crate) fn evaluate( BinaryOp::Sub { max_rhs_value } | BinaryOp::SafeSub { max_rhs_value } => { let l_c = var_cache.get_or_compute_internal_var_unwrap(binary.lhs, evaluator, ctx); let r_c = var_cache.get_or_compute_internal_var_unwrap(binary.rhs, evaluator, ctx); - if res_type == ObjectType::NativeField { + if res_type.is_native_field() { InternalVar::from(constraints::subtract( l_c.expression(), FieldElement::one(), diff --git a/crates/noirc_evaluator/src/ssa/builtin.rs b/crates/noirc_evaluator/src/ssa/builtin.rs index f3ee01f98be..15397c08a82 100644 --- a/crates/noirc_evaluator/src/ssa/builtin.rs +++ b/crates/noirc_evaluator/src/ssa/builtin.rs @@ -55,7 +55,7 @@ impl Opcode { BlackBoxFunc::SchnorrVerify | BlackBoxFunc::EcdsaSecp256k1 | BlackBoxFunc::MerkleMembership => BigUint::one(), - BlackBoxFunc::HashToField128Security => ObjectType::NativeField.max_size(), + BlackBoxFunc::HashToField128Security => ObjectType::native_field().max_size(), BlackBoxFunc::AES => { todo!("ICE: AES is unimplemented") } @@ -75,21 +75,23 @@ impl Opcode { Opcode::LowLevel(op) => { match op { BlackBoxFunc::AES => todo!("ICE: AES is unimplemented"), - BlackBoxFunc::SHA256 | BlackBoxFunc::Blake2s => (32, ObjectType::Unsigned(8)), - BlackBoxFunc::HashToField128Security => (1, ObjectType::NativeField), + BlackBoxFunc::SHA256 | BlackBoxFunc::Blake2s => { + (32, ObjectType::unsigned_integer(8)) + } + BlackBoxFunc::HashToField128Security => (1, ObjectType::native_field()), // See issue #775 on changing this to return a boolean BlackBoxFunc::MerkleMembership | BlackBoxFunc::SchnorrVerify - | BlackBoxFunc::EcdsaSecp256k1 => (1, ObjectType::NativeField), - BlackBoxFunc::Pedersen => (2, ObjectType::NativeField), - BlackBoxFunc::FixedBaseScalarMul => (2, ObjectType::NativeField), + | BlackBoxFunc::EcdsaSecp256k1 => (1, ObjectType::native_field()), + BlackBoxFunc::Pedersen => (2, ObjectType::native_field()), + BlackBoxFunc::FixedBaseScalarMul => (2, ObjectType::native_field()), BlackBoxFunc::RANGE | BlackBoxFunc::AND | BlackBoxFunc::XOR => { unreachable!("ICE: these opcodes do not have Noir builtin functions") } } } Opcode::ToBits => (FieldElement::max_num_bits(), ObjectType::Boolean), - Opcode::ToRadix => (FieldElement::max_num_bits(), ObjectType::NativeField), + Opcode::ToRadix => (FieldElement::max_num_bits(), ObjectType::native_field()), } } } diff --git a/crates/noirc_evaluator/src/ssa/code_gen.rs b/crates/noirc_evaluator/src/ssa/code_gen.rs index cb3186a0e91..ecad669853e 100644 --- a/crates/noirc_evaluator/src/ssa/code_gen.rs +++ b/crates/noirc_evaluator/src/ssa/code_gen.rs @@ -153,10 +153,10 @@ impl IRGenerator { pub fn get_object_type_from_abi(&self, el_type: &noirc_abi::AbiType) -> ObjectType { match el_type { - noirc_abi::AbiType::Field => ObjectType::NativeField, + noirc_abi::AbiType::Field => ObjectType::native_field(), noirc_abi::AbiType::Integer { sign, width, .. } => match sign { - noirc_abi::Sign::Unsigned => ObjectType::Unsigned(*width), - noirc_abi::Sign::Signed => ObjectType::Signed(*width), + noirc_abi::Sign::Unsigned => ObjectType::unsigned_integer(*width), + noirc_abi::Sign::Signed => ObjectType::signed_integer(*width), }, noirc_abi::AbiType::Boolean => ObjectType::Boolean, noirc_abi::AbiType::Array { .. } => { @@ -383,7 +383,8 @@ impl IRGenerator { Value::Single(v_id) } Type::String(len) => { - let obj_type = ObjectType::Unsigned(8); + // TODO: document why this is 8 + let obj_type = ObjectType::unsigned_integer(8); let len = *len; let (v_id, _) = self.new_array(base_name, obj_type, len.try_into().unwrap(), def); Value::Single(v_id) @@ -571,7 +572,7 @@ impl IRGenerator { for (pos, object) in elements.into_iter().enumerate() { let lhs_adr = self.context.get_or_create_const( FieldElement::from((pos as u32) as u128), - ObjectType::NativeField, + ObjectType::native_field(), ); let store = Operation::Store { array_id, index: lhs_adr, value: object }; self.context.new_instruction(store, element_type)?; diff --git a/crates/noirc_evaluator/src/ssa/context.rs b/crates/noirc_evaluator/src/ssa/context.rs index f01cc4d1e2c..02d6a34548e 100644 --- a/crates/noirc_evaluator/src/ssa/context.rs +++ b/crates/noirc_evaluator/src/ssa/context.rs @@ -792,10 +792,14 @@ impl SsaContext { let len = self.mem[a].len; let e_type = self.mem[b].element_type; for i in 0..len { - let idx_b = self - .get_or_create_const(FieldElement::from(i as i128), ObjectType::Unsigned(32)); - let idx_a = self - .get_or_create_const(FieldElement::from(i as i128), ObjectType::Unsigned(32)); + let idx_b = self.get_or_create_const( + FieldElement::from(i as i128), + ObjectType::unsigned_integer(32), + ); + let idx_a = self.get_or_create_const( + FieldElement::from(i as i128), + ObjectType::unsigned_integer(32), + ); let op_b = Operation::Load { array_id: b, index: idx_b }; let load = self.new_instruction(op_b, e_type)?; let op_a = Operation::Store { array_id: a, index: idx_a, value: load }; @@ -937,8 +941,10 @@ impl SsaContext { let len = self.mem[array_id].len; let e_type = self.mem[array_id].element_type; for i in 0..len { - let index = - self.get_or_create_const(FieldElement::from(i as i128), ObjectType::Unsigned(32)); + let index = self.get_or_create_const( + FieldElement::from(i as i128), + ObjectType::unsigned_integer(32), + ); let op_a = Operation::Store { array_id, index, value: self.zero_with_type(e_type) }; self.new_instruction_inline(op_a, e_type, stack_frame); } @@ -958,10 +964,15 @@ impl SsaContext { let len = self.mem[a].len; let e_type = self.mem[b].element_type; for i in 0..len { - let idx_b = self - .get_or_create_const(FieldElement::from(i as i128), ObjectType::Unsigned(32)); - let idx_a = self - .get_or_create_const(FieldElement::from(i as i128), ObjectType::Unsigned(32)); + let idx_b = self.get_or_create_const( + FieldElement::from(i as i128), + // TODO: Should document where 32 comes from + ObjectType::unsigned_integer(32), + ); + let idx_a = self.get_or_create_const( + FieldElement::from(i as i128), + ObjectType::unsigned_integer(32), + ); let op_b = Operation::Load { array_id: b, index: idx_b }; let load = self.new_instruction_inline(op_b, e_type, stack_frame); let op_a = Operation::Store { array_id: a, index: idx_a, value: load }; @@ -1050,7 +1061,7 @@ impl SsaContext { let (id, array_id) = self.new_array(&name, el_type, len, None); for i in 0..len { let index = self - .get_or_create_const(FieldElement::from(i as u128), ObjectType::NativeField); + .get_or_create_const(FieldElement::from(i as u128), ObjectType::native_field()); self.current_block = block1; let op = Operation::Load { array_id: adr1, index }; let v1 = self.new_instruction(op, el_type).unwrap(); @@ -1120,15 +1131,15 @@ impl SsaContext { use noirc_frontend::Signedness; match t { Type::Bool => ObjectType::Boolean, - Type::Field => ObjectType::NativeField, + Type::Field => ObjectType::native_field(), Type::Integer(sign, bit_size) => { assert!( *bit_size < super::integer::short_integer_max_bit_size(), "long integers are not yet supported" ); match sign { - Signedness::Signed => ObjectType::Signed(*bit_size), - Signedness::Unsigned => ObjectType::Unsigned(*bit_size), + Signedness::Signed => ObjectType::signed_integer(*bit_size), + Signedness::Unsigned => ObjectType::unsigned_integer(*bit_size), } } Type::Array(..) => panic!("Cannot convert an array type {t} into an ObjectType since it is unknown which array it refers to"), diff --git a/crates/noirc_evaluator/src/ssa/integer.rs b/crates/noirc_evaluator/src/ssa/integer.rs index 20e3b5a26fa..9bd1b391a3c 100644 --- a/crates/noirc_evaluator/src/ssa/integer.rs +++ b/crates/noirc_evaluator/src/ssa/integer.rs @@ -54,7 +54,7 @@ fn get_instruction_max_operand( Operation::Binary(node::Binary { operator, lhs, rhs, .. }) => { if let BinaryOp::Sub { .. } = operator { //TODO uses interval analysis instead - if matches!(ins.res_type, ObjectType::Unsigned(_)) { + if ins.res_type.is_unsigned_integer() { if let Some(lhs_const) = ctx.get_as_constant(*lhs) { let lhs_big = BigUint::from_bytes_be(&lhs_const.to_be_bytes()); if max_map[rhs] <= lhs_big { @@ -266,14 +266,14 @@ fn block_overflow( let ins_max_bits = get_instruction_max(ctx, &ins, max_map, &value_map).bits(); let res_type = ins.res_type; - let too_many_bits = ins_max_bits > FieldElement::max_num_bits() as u64 - && res_type != ObjectType::NativeField; + let too_many_bits = + ins_max_bits > FieldElement::max_num_bits() as u64 && !res_type.is_native_field(); ins.operation.for_each_id(|id| { get_obj_max_value(ctx, id, max_map, &value_map); let arg = ctx.try_get_node(id); let should_truncate_arg = - should_truncate_ins && arg.is_some() && get_type(arg) != ObjectType::NativeField; + should_truncate_ins && arg.is_some() && !get_type(arg).is_native_field(); if should_truncate_arg || too_many_bits { add_to_truncate(ctx, id, get_size_in_bits(arg), &mut truncate_map, max_map); @@ -308,7 +308,7 @@ fn block_overflow( } } Operation::Binary(node::Binary { operator: BinaryOp::Shr, lhs, rhs, .. }) => { - if !matches!(ins.res_type, node::ObjectType::Unsigned(_)) { + if !ins.res_type.is_unsigned_integer() { todo!("Right shift is only implemented for unsigned integers"); } if let Some(r_const) = ctx.get_as_constant(rhs) { @@ -490,7 +490,7 @@ fn get_max_value(ins: &Instruction, max_map: &mut HashMap) -> B Operation::Intrinsic(opcode, _) => opcode.get_max_value(), }; - if ins.res_type == ObjectType::NativeField { + if ins.res_type.is_native_field() { let field_max = BigUint::from_bytes_be(&FieldElement::one().neg().to_be_bytes()); //Native Field operations cannot overflow so they will not be truncated diff --git a/crates/noirc_evaluator/src/ssa/node.rs b/crates/noirc_evaluator/src/ssa/node.rs index a8991a480d5..c726a9821a9 100644 --- a/crates/noirc_evaluator/src/ssa/node.rs +++ b/crates/noirc_evaluator/src/ssa/node.rs @@ -173,28 +173,25 @@ impl Variable { #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] pub enum ObjectType { - NativeField, Boolean, - Unsigned(u32), //bit size - Signed(u32), //bit size + // TODO: Why don't we put Boolean under Numeric type (u1)? + Numeric(NumericType), Pointer(ArrayId), Function, NotAnObject, //not an object } -#[derive(Copy, Clone, Debug, PartialEq, Eq)] +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] pub enum NumericType { - Signed(u32), - Unsigned(u32), + Signed(u32), // bit size + Unsigned(u32), // bit size NativeField, } impl From for NumericType { fn from(object_type: ObjectType) -> NumericType { match object_type { - ObjectType::Signed(x) => NumericType::Signed(x), - ObjectType::Unsigned(x) => NumericType::Unsigned(x), - ObjectType::NativeField => NumericType::NativeField, + ObjectType::Numeric(numeric_type) => numeric_type, _ => unreachable!("failed to convert an object type into a numeric type"), } } @@ -204,33 +201,66 @@ impl ObjectType { pub fn bits(&self) -> u32 { match self { ObjectType::Boolean => 1, - ObjectType::NativeField => FieldElement::max_num_bits(), ObjectType::NotAnObject => 0, - ObjectType::Signed(c) => *c, - ObjectType::Unsigned(c) => *c, ObjectType::Pointer(_) => 0, ObjectType::Function => 0, + ObjectType::Numeric(numeric_type) => match numeric_type { + NumericType::Signed(c) | NumericType::Unsigned(c) => *c, + NumericType::NativeField => FieldElement::max_num_bits(), + }, } } + /// Returns the type that represents + /// a field element. + /// The most basic/fundamental type in the language + pub fn native_field() -> ObjectType { + ObjectType::Numeric(NumericType::NativeField) + } + /// Returns a type that represents an unsigned integer + pub fn unsigned_integer(bit_size: u32) -> ObjectType { + ObjectType::Numeric(NumericType::Unsigned(bit_size)) + } + /// Returns a type that represents an signed integer + pub fn signed_integer(bit_size: u32) -> ObjectType { + ObjectType::Numeric(NumericType::Signed(bit_size)) + } + + /// Returns true, if the `ObjectType` + /// represents a field element + pub fn is_native_field(&self) -> bool { + matches!(self, ObjectType::Numeric(NumericType::NativeField)) + } + /// Returns true, if the `ObjectType` + /// represents an unsigned integer + pub fn is_unsigned_integer(&self) -> bool { + matches!(self, ObjectType::Numeric(NumericType::Unsigned(_))) + } + //maximum size of the representation (e.g. signed(8).max_size() return 255, not 128.) pub fn max_size(&self) -> BigUint { match self { - &ObjectType::NativeField => { + ObjectType::Numeric(NumericType::NativeField) => { BigUint::from_bytes_be(&FieldElement::from(-1_i128).to_be_bytes()) } _ => (BigUint::one() << self.bits()) - BigUint::one(), } } - + // TODO: the name of this function is misleading + // TODO since the type is not being returned pub fn field_to_type(&self, f: FieldElement) -> FieldElement { match self { + // TODO: document why this is unreachable ObjectType::NotAnObject | ObjectType::Pointer(_) => { unreachable!() } - ObjectType::NativeField => f, - ObjectType::Signed(_) => todo!(), - _ => { + ObjectType::Numeric(NumericType::NativeField) => f, + // TODO: document why this is a TODO and create an issue + ObjectType::Numeric(NumericType::Signed(_)) => todo!(), + ObjectType::Boolean + | ObjectType::Function + | ObjectType::Numeric(NumericType::Unsigned(_)) => { + // TODO: document where this 128 comes from assert!(self.bits() < 128); FieldElement::from(f.to_u128() % (1_u128 << self.bits())) } @@ -378,7 +408,7 @@ impl Instruction { } Operation::Cast(value) => { if let Some(l_const) = eval_fn(ctx, *value)?.into_const_value() { - if self.res_type == ObjectType::NativeField { + if self.res_type.is_native_field() { return Ok(NodeEval::Const(l_const, self.res_type)); } else if let Some(l_const) = l_const.try_into_u128() { return Ok(NodeEval::Const( @@ -829,7 +859,10 @@ impl Binary { return Ok(NodeEval::Const(FieldElement::zero(), ObjectType::Boolean)); //n.b we assume the type of lhs and rhs is unsigned because of the opcode, we could also verify this } else if let (Some(lhs), Some(rhs)) = (lhs, rhs) { - assert_ne!(res_type, ObjectType::NativeField); //comparisons are not implemented for field elements + assert!( + !res_type.is_native_field(), + "ICE: comparisons are not implemented for field elements" + ); let res = if lhs < rhs { FieldElement::one() } else { FieldElement::zero() }; return Ok(NodeEval::Const(res, ObjectType::Boolean)); } @@ -839,7 +872,10 @@ impl Binary { return Ok(NodeEval::Const(FieldElement::one(), ObjectType::Boolean)); //n.b we assume the type of lhs and rhs is unsigned because of the opcode, we could also verify this } else if let (Some(lhs), Some(rhs)) = (lhs, rhs) { - assert_ne!(res_type, ObjectType::NativeField); //comparisons are not implemented for field elements + assert!( + !res_type.is_native_field(), + "ICE: comparisons are not implemented for field elements" + ); let res = if lhs <= rhs { FieldElement::one() } else { FieldElement::zero() }; return Ok(NodeEval::Const(res, ObjectType::Boolean)); } @@ -1028,7 +1064,7 @@ fn wrapping( u128_op: impl FnOnce(u128, u128) -> u128, field_op: impl FnOnce(FieldElement, FieldElement) -> FieldElement, ) -> NodeEval { - if res_type != ObjectType::NativeField { + if !res_type.is_native_field() { let type_modulo = 1_u128 << res_type.bits(); let lhs = lhs.to_u128() % type_modulo; let rhs = rhs.to_u128() % type_modulo; @@ -1239,7 +1275,8 @@ impl BinaryOp { ) } } - +// TODO: We should create a constant and explain where the 127 and 126 constants +// TODO are from fn field_to_signed(f: FieldElement, n: u32) -> i128 { assert!(n < 127); let a = f.to_u128(); diff --git a/crates/noirc_evaluator/src/ssa/optimizations.rs b/crates/noirc_evaluator/src/ssa/optimizations.rs index 182410df7d7..cf4e8866338 100644 --- a/crates/noirc_evaluator/src/ssa/optimizations.rs +++ b/crates/noirc_evaluator/src/ssa/optimizations.rs @@ -84,7 +84,7 @@ fn evaluate_intrinsic( for i in 0..bit_count { let index = ctx.get_or_create_const( FieldElement::from(i as i128), - ObjectType::NativeField, + ObjectType::native_field(), ); let op = if args[0] & (1 << i) != 0 { Operation::Store { array_id: *a, index, value: ctx.one() } From 12289dbd325cbad757e9d73ace1895901881d3db Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Sat, 11 Feb 2023 16:39:58 +0000 Subject: [PATCH 2/5] fix import - submodules were using `node` path from the root --- crates/noirc_evaluator/src/lib.rs | 6 +++--- crates/noirc_evaluator/src/ssa/anchor.rs | 2 +- crates/noirc_evaluator/src/ssa/conditional.rs | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/noirc_evaluator/src/lib.rs b/crates/noirc_evaluator/src/lib.rs index 16dbba2f366..8b6479f47ed 100644 --- a/crates/noirc_evaluator/src/lib.rs +++ b/crates/noirc_evaluator/src/lib.rs @@ -11,7 +11,7 @@ use errors::{RuntimeError, RuntimeErrorKind}; use iter_extended::btree_map; use noirc_abi::{AbiType, AbiVisibility}; use noirc_frontend::monomorphization::ast::*; -use ssa::{code_gen::IRGenerator, node}; +use ssa::{code_gen::IRGenerator, node::ObjectType}; use std::collections::BTreeMap; pub struct Evaluator { @@ -151,7 +151,7 @@ impl Evaluator { ir_gen.create_new_variable( name.to_owned(), Some(def), - node::ObjectType::native_field(), + ObjectType::native_field(), Some(witness), ); } @@ -177,7 +177,7 @@ impl Evaluator { if *visibility == AbiVisibility::Public { self.public_inputs.push(witness); } - let obj_type = node::ObjectType::Boolean; + let obj_type = ObjectType::Boolean; ir_gen.create_new_variable(name.to_owned(), Some(def), obj_type, Some(witness)); } AbiType::Struct { fields } => { diff --git a/crates/noirc_evaluator/src/ssa/anchor.rs b/crates/noirc_evaluator/src/ssa/anchor.rs index 2658e36b0b5..dc30556a05a 100644 --- a/crates/noirc_evaluator/src/ssa/anchor.rs +++ b/crates/noirc_evaluator/src/ssa/anchor.rs @@ -24,7 +24,7 @@ pub enum CseAction { #[derive(Default, Clone)] pub struct Anchor { map: HashMap>, //standard anchor - cast_map: HashMap>, //cast anchor + cast_map: HashMap>, //cast anchor mem_map: HashMap>>, //Memory anchor: one Vec for each array where Vec[i] contains the list of load and store instructions having index i, and the mem_item position in which they appear mem_list: HashMap>, // list of the memory instructions, per array, and grouped into MemItems } diff --git a/crates/noirc_evaluator/src/ssa/conditional.rs b/crates/noirc_evaluator/src/ssa/conditional.rs index 1479a2487ff..45a05228f63 100644 --- a/crates/noirc_evaluator/src/ssa/conditional.rs +++ b/crates/noirc_evaluator/src/ssa/conditional.rs @@ -3,7 +3,7 @@ use crate::ssa::{ context::SsaContext, flatten::UnrollContext, inline::StackFrame, - node::{BinaryOp, Instruction, Mark, NodeId, ObjectType, Opcode, Operation}, + node::{Binary, BinaryOp, Instruction, Mark, NodeId, ObjectType, Opcode, Operation}, {block, flatten, node, optimizations}, }; use crate::{errors, errors::RuntimeError}; @@ -605,7 +605,7 @@ impl DecisionTree { } if ctx.under_assumption(cond) { let ins2 = ctx.get_mut_instruction(ins_id); - ins2.operation = Operation::Binary(crate::node::Binary { + ins2.operation = Operation::Binary(Binary { lhs: binary_op.lhs, rhs: binary_op.rhs, operator: binary_op.operator.clone(), From 074176826461cad2a2190a17fc2792a8e8f11499 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Tue, 14 Feb 2023 13:16:39 +0000 Subject: [PATCH 3/5] boolean is now seen as `u1` --- crates/noirc_evaluator/src/lib.rs | 2 +- .../src/ssa/acir_gen/internal_var_cache.rs | 2 +- crates/noirc_evaluator/src/ssa/builtin.rs | 2 +- crates/noirc_evaluator/src/ssa/code_gen.rs | 4 +- crates/noirc_evaluator/src/ssa/conditional.rs | 12 +++--- crates/noirc_evaluator/src/ssa/context.rs | 22 +++++----- crates/noirc_evaluator/src/ssa/node.rs | 41 ++++++++++--------- 7 files changed, 43 insertions(+), 42 deletions(-) diff --git a/crates/noirc_evaluator/src/lib.rs b/crates/noirc_evaluator/src/lib.rs index 8b6479f47ed..a7b1fa9409c 100644 --- a/crates/noirc_evaluator/src/lib.rs +++ b/crates/noirc_evaluator/src/lib.rs @@ -177,7 +177,7 @@ impl Evaluator { if *visibility == AbiVisibility::Public { self.public_inputs.push(witness); } - let obj_type = ObjectType::Boolean; + let obj_type = ObjectType::boolean(); ir_gen.create_new_variable(name.to_owned(), Some(def), obj_type, Some(witness)); } AbiType::Struct { fields } => { diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/internal_var_cache.rs b/crates/noirc_evaluator/src/ssa/acir_gen/internal_var_cache.rs index 08d6d1a6829..130d04faa30 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/internal_var_cache.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/internal_var_cache.rs @@ -37,7 +37,7 @@ impl InternalVarCache { NodeObject::Obj(variable) => { let variable_type = variable.get_type(); match variable_type { - ObjectType::Boolean | ObjectType::Numeric(..) => { + ObjectType::Numeric(..) => { let witness = variable.witness.unwrap_or_else(|| evaluator.add_witness_to_cs()); InternalVar::from_witness(witness) diff --git a/crates/noirc_evaluator/src/ssa/builtin.rs b/crates/noirc_evaluator/src/ssa/builtin.rs index 15397c08a82..c81f2b2c983 100644 --- a/crates/noirc_evaluator/src/ssa/builtin.rs +++ b/crates/noirc_evaluator/src/ssa/builtin.rs @@ -90,7 +90,7 @@ impl Opcode { } } } - Opcode::ToBits => (FieldElement::max_num_bits(), ObjectType::Boolean), + Opcode::ToBits => (FieldElement::max_num_bits(), ObjectType::boolean()), Opcode::ToRadix => (FieldElement::max_num_bits(), ObjectType::native_field()), } } diff --git a/crates/noirc_evaluator/src/ssa/code_gen.rs b/crates/noirc_evaluator/src/ssa/code_gen.rs index ecad669853e..36171a6c840 100644 --- a/crates/noirc_evaluator/src/ssa/code_gen.rs +++ b/crates/noirc_evaluator/src/ssa/code_gen.rs @@ -158,7 +158,7 @@ impl IRGenerator { noirc_abi::Sign::Unsigned => ObjectType::unsigned_integer(*width), noirc_abi::Sign::Signed => ObjectType::signed_integer(*width), }, - noirc_abi::AbiType::Boolean => ObjectType::Boolean, + noirc_abi::AbiType::Boolean => ObjectType::boolean(), noirc_abi::AbiType::Array { .. } => { unreachable!("array of arrays are not supported for now") } @@ -734,7 +734,7 @@ impl IRGenerator { self.update_variable_id(iter_id, iter_id, phi); //is it still needed? let not_equal = Operation::binary(BinaryOp::Ne, phi, end_idx); - let cond = self.context.new_instruction(not_equal, ObjectType::Boolean)?; + let cond = self.context.new_instruction(not_equal, ObjectType::boolean())?; let to_fix = self.context.new_instruction(Operation::Nop, ObjectType::NotAnObject)?; diff --git a/crates/noirc_evaluator/src/ssa/conditional.rs b/crates/noirc_evaluator/src/ssa/conditional.rs index 45a05228f63..fdd44f54a8c 100644 --- a/crates/noirc_evaluator/src/ssa/conditional.rs +++ b/crates/noirc_evaluator/src/ssa/conditional.rs @@ -168,7 +168,7 @@ impl DecisionTree { BinaryOp::Mul, parent_value, condition, - ObjectType::Boolean, + ObjectType::boolean(), ) } else { let not_condition = DecisionTree::new_instruction_after_phi( @@ -177,7 +177,7 @@ impl DecisionTree { BinaryOp::Sub { max_rhs_value: BigUint::one() }, ctx.one(), condition, - ObjectType::Boolean, + ObjectType::boolean(), ); DecisionTree::new_instruction_after( ctx, @@ -185,7 +185,7 @@ impl DecisionTree { BinaryOp::Mul, parent_value, not_condition, - ObjectType::Boolean, + ObjectType::boolean(), not_condition, ) }; @@ -468,7 +468,7 @@ impl DecisionTree { Operation::Cond { condition, val_true: ctx.zero(), val_false: ctx.one() }; let cond = ctx.add_instruction(Instruction::new( operation, - ObjectType::Boolean, + ObjectType::boolean(), Some(stack.block), )); stack.push(cond); @@ -580,7 +580,7 @@ impl DecisionTree { }); cond = ctx.add_instruction(Instruction::new( op, - ObjectType::Boolean, + ObjectType::boolean(), Some(stack.block), )); optimizations::simplify_id(ctx, cond).unwrap(); @@ -722,7 +722,7 @@ impl DecisionTree { } let cond = ctx.add_instruction(Instruction::new( operation, - ObjectType::Boolean, + ObjectType::boolean(), Some(stack.block), )); stack.push(cond); diff --git a/crates/noirc_evaluator/src/ssa/context.rs b/crates/noirc_evaluator/src/ssa/context.rs index 02d6a34548e..32c26611a33 100644 --- a/crates/noirc_evaluator/src/ssa/context.rs +++ b/crates/noirc_evaluator/src/ssa/context.rs @@ -58,17 +58,17 @@ impl SsaContext { dummy_load: HashMap::new(), }; block::create_first_block(&mut pc); - pc.one_with_type(node::ObjectType::Boolean); - pc.zero_with_type(node::ObjectType::Boolean); + pc.one_with_type(ObjectType::boolean()); + pc.zero_with_type(ObjectType::boolean()); pc } pub fn zero(&self) -> NodeId { - self.find_const_with_type(&BigUint::zero(), node::ObjectType::Boolean).unwrap() + self.find_const_with_type(&BigUint::zero(), ObjectType::boolean()).unwrap() } pub fn one(&self) -> NodeId { - self.find_const_with_type(&BigUint::one(), node::ObjectType::Boolean).unwrap() + self.find_const_with_type(&BigUint::one(), ObjectType::boolean()).unwrap() } pub fn zero_with_type(&mut self, obj_type: ObjectType) -> NodeId { @@ -125,7 +125,7 @@ impl SsaContext { if !self.dummy_store.contains_key(&a) { let op_a = Operation::Store { array_id: a, index: NodeId::dummy(), value: NodeId::dummy() }; - let dummy_store = node::Instruction::new(op_a, node::ObjectType::NotAnObject, None); + let dummy_store = node::Instruction::new(op_a, ObjectType::NotAnObject, None); let id = self.add_instruction(dummy_store); self.dummy_store.insert(a, id); } @@ -615,7 +615,7 @@ impl SsaContext { | Binary(node::Binary { operator: Slt, .. }) | Binary(node::Binary { operator: Sle, .. }) | Binary(node::Binary { operator: Lt, .. }) - | Binary(node::Binary { operator: Lte, .. }) => ObjectType::Boolean, + | Binary(node::Binary { operator: Lte, .. }) => ObjectType::boolean(), Operation::Jne(_, _) | Operation::Jeq(_, _) | Operation::Jmp(_) @@ -643,7 +643,7 @@ impl SsaContext { //we create a variable pointing to this MemArray let new_var = node::Variable { id: NodeId::dummy(), - obj_type: node::ObjectType::Pointer(array_index), + obj_type: ObjectType::Pointer(array_index), name: name.to_string(), root: None, def: def.clone(), @@ -1055,7 +1055,7 @@ impl SsaContext { let name = format!("if_{}_ret{c}", exit_block.0.into_raw_parts().0); *c += 1; - if let node::ObjectType::Pointer(adr1) = a_type { + if let ObjectType::Pointer(adr1) = a_type { let len = self.mem[adr1].len; let el_type = self.mem[adr1].element_type; let (id, array_id) = self.new_array(&name, el_type, len, None); @@ -1130,7 +1130,7 @@ impl SsaContext { use noirc_frontend::monomorphization::ast::Type; use noirc_frontend::Signedness; match t { - Type::Bool => ObjectType::Boolean, + Type::Bool => ObjectType::boolean(), Type::Field => ObjectType::native_field(), Type::Integer(sign, bit_size) => { assert!( @@ -1174,7 +1174,7 @@ impl SsaContext { }); let cond = self.add_instruction(Instruction::new( op, - ObjectType::Boolean, + ObjectType::boolean(), Some(stack.block), )); optimizations::simplify_id(self, cond).unwrap(); @@ -1191,7 +1191,7 @@ impl SsaContext { Operation::Cond { condition: pred, val_true: *cond, val_false: self.one() }; let c_ins = self.add_instruction(Instruction::new( operation, - ObjectType::Boolean, + ObjectType::boolean(), Some(stack.block), )); stack.push(c_ins); diff --git a/crates/noirc_evaluator/src/ssa/node.rs b/crates/noirc_evaluator/src/ssa/node.rs index c726a9821a9..b9d9b65ef80 100644 --- a/crates/noirc_evaluator/src/ssa/node.rs +++ b/crates/noirc_evaluator/src/ssa/node.rs @@ -173,8 +173,6 @@ impl Variable { #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] pub enum ObjectType { - Boolean, - // TODO: Why don't we put Boolean under Numeric type (u1)? Numeric(NumericType), Pointer(ArrayId), Function, @@ -200,7 +198,6 @@ impl From for NumericType { impl ObjectType { pub fn bits(&self) -> u32 { match self { - ObjectType::Boolean => 1, ObjectType::NotAnObject => 0, ObjectType::Pointer(_) => 0, ObjectType::Function => 0, @@ -221,6 +218,12 @@ impl ObjectType { pub fn unsigned_integer(bit_size: u32) -> ObjectType { ObjectType::Numeric(NumericType::Unsigned(bit_size)) } + /// Returns a type that represents an boolean + /// Booleans are just seen as an unsigned integer + /// with a bit size of 1. + pub fn boolean() -> ObjectType { + ObjectType::unsigned_integer(1) + } /// Returns a type that represents an signed integer pub fn signed_integer(bit_size: u32) -> ObjectType { ObjectType::Numeric(NumericType::Signed(bit_size)) @@ -257,9 +260,7 @@ impl ObjectType { ObjectType::Numeric(NumericType::NativeField) => f, // TODO: document why this is a TODO and create an issue ObjectType::Numeric(NumericType::Signed(_)) => todo!(), - ObjectType::Boolean - | ObjectType::Function - | ObjectType::Numeric(NumericType::Unsigned(_)) => { + ObjectType::Function | ObjectType::Numeric(NumericType::Unsigned(_)) => { // TODO: document where this 128 comes from assert!(self.bits() < 128); FieldElement::from(f.to_u128() % (1_u128 << self.bits())) @@ -856,7 +857,7 @@ impl Binary { } BinaryOp::Ult => { if r_is_zero { - return Ok(NodeEval::Const(FieldElement::zero(), ObjectType::Boolean)); + return Ok(NodeEval::Const(FieldElement::zero(), ObjectType::boolean())); //n.b we assume the type of lhs and rhs is unsigned because of the opcode, we could also verify this } else if let (Some(lhs), Some(rhs)) = (lhs, rhs) { assert!( @@ -864,12 +865,12 @@ impl Binary { "ICE: comparisons are not implemented for field elements" ); let res = if lhs < rhs { FieldElement::one() } else { FieldElement::zero() }; - return Ok(NodeEval::Const(res, ObjectType::Boolean)); + return Ok(NodeEval::Const(res, ObjectType::boolean())); } } BinaryOp::Ule => { if l_is_zero { - return Ok(NodeEval::Const(FieldElement::one(), ObjectType::Boolean)); + return Ok(NodeEval::Const(FieldElement::one(), ObjectType::boolean())); //n.b we assume the type of lhs and rhs is unsigned because of the opcode, we could also verify this } else if let (Some(lhs), Some(rhs)) = (lhs, rhs) { assert!( @@ -877,48 +878,48 @@ impl Binary { "ICE: comparisons are not implemented for field elements" ); let res = if lhs <= rhs { FieldElement::one() } else { FieldElement::zero() }; - return Ok(NodeEval::Const(res, ObjectType::Boolean)); + return Ok(NodeEval::Const(res, ObjectType::boolean())); } } BinaryOp::Slt => (), BinaryOp::Sle => (), BinaryOp::Lt => { if r_is_zero { - return Ok(NodeEval::Const(FieldElement::zero(), ObjectType::Boolean)); + return Ok(NodeEval::Const(FieldElement::zero(), ObjectType::boolean())); //n.b we assume the type of lhs and rhs is unsigned because of the opcode, we could also verify this } else if let (Some(lhs), Some(rhs)) = (lhs, rhs) { let res = if lhs < rhs { FieldElement::one() } else { FieldElement::zero() }; - return Ok(NodeEval::Const(res, ObjectType::Boolean)); + return Ok(NodeEval::Const(res, ObjectType::boolean())); } } BinaryOp::Lte => { if l_is_zero { - return Ok(NodeEval::Const(FieldElement::one(), ObjectType::Boolean)); + return Ok(NodeEval::Const(FieldElement::one(), ObjectType::boolean())); //n.b we assume the type of lhs and rhs is unsigned because of the opcode, we could also verify this } else if let (Some(lhs), Some(rhs)) = (lhs, rhs) { let res = if lhs <= rhs { FieldElement::one() } else { FieldElement::zero() }; - return Ok(NodeEval::Const(res, ObjectType::Boolean)); + return Ok(NodeEval::Const(res, ObjectType::boolean())); } } BinaryOp::Eq => { if self.lhs == self.rhs { - return Ok(NodeEval::Const(FieldElement::one(), ObjectType::Boolean)); + return Ok(NodeEval::Const(FieldElement::one(), ObjectType::boolean())); } else if let (Some(lhs), Some(rhs)) = (lhs, rhs) { if lhs == rhs { - return Ok(NodeEval::Const(FieldElement::one(), ObjectType::Boolean)); + return Ok(NodeEval::Const(FieldElement::one(), ObjectType::boolean())); } else { - return Ok(NodeEval::Const(FieldElement::zero(), ObjectType::Boolean)); + return Ok(NodeEval::Const(FieldElement::zero(), ObjectType::boolean())); } } } BinaryOp::Ne => { if self.lhs == self.rhs { - return Ok(NodeEval::Const(FieldElement::zero(), ObjectType::Boolean)); + return Ok(NodeEval::Const(FieldElement::zero(), ObjectType::boolean())); } else if let (Some(lhs), Some(rhs)) = (lhs, rhs) { if lhs != rhs { - return Ok(NodeEval::Const(FieldElement::one(), ObjectType::Boolean)); + return Ok(NodeEval::Const(FieldElement::one(), ObjectType::boolean())); } else { - return Ok(NodeEval::Const(FieldElement::zero(), ObjectType::Boolean)); + return Ok(NodeEval::Const(FieldElement::zero(), ObjectType::boolean())); } } } From 90507aa12e4badd334210dacb27461ce184a9fba Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Tue, 14 Feb 2023 13:18:12 +0000 Subject: [PATCH 4/5] add comment re strings --- crates/noirc_evaluator/src/ssa/code_gen.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa/code_gen.rs b/crates/noirc_evaluator/src/ssa/code_gen.rs index 36171a6c840..fe1a1f3688b 100644 --- a/crates/noirc_evaluator/src/ssa/code_gen.rs +++ b/crates/noirc_evaluator/src/ssa/code_gen.rs @@ -383,7 +383,7 @@ impl IRGenerator { Value::Single(v_id) } Type::String(len) => { - // TODO: document why this is 8 + // Strings are a packed array of utf-8 encoded bytes let obj_type = ObjectType::unsigned_integer(8); let len = *len; let (v_id, _) = self.new_array(base_name, obj_type, len.try_into().unwrap(), def); From 1d49103b708d86bb79939aa37b04c6d8d89c3215 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Tue, 14 Feb 2023 18:59:37 +0000 Subject: [PATCH 5/5] Merge remote-tracking branch 'origin/master' into kw/dedup-numeric-type --- .github/workflows/release.yml | 5 +- Cargo.lock | 18 +-- crates/nargo/src/cli/mod.rs | 7 +- crates/nargo/src/cli/test_cmd.rs | 17 ++- .../nargo/tests/test_data/strings/Prover.toml | 3 +- .../nargo/tests/test_data/strings/src/main.nr | 46 ++++++- crates/noirc_abi/src/lib.rs | 31 +++-- crates/noirc_driver/src/lib.rs | 5 +- crates/noirc_evaluator/src/lib.rs | 6 +- crates/noirc_evaluator/src/ssa/acir_gen.rs | 13 +- .../src/ssa/acir_gen/internal_var.rs | 3 + .../src/ssa/acir_gen/operations/intrinsics.rs | 120 +++++++++++++++++- crates/noirc_evaluator/src/ssa/builtin.rs | 17 ++- crates/noirc_evaluator/src/ssa/context.rs | 48 +++++-- crates/noirc_evaluator/src/ssa/function.rs | 2 +- .../noirc_evaluator/src/ssa/optimizations.rs | 39 ++++-- crates/noirc_frontend/src/ast/statement.rs | 1 - noir_stdlib/src/lib.nr | 3 + 18 files changed, 315 insertions(+), 69 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 26b826ed4c8..f92f06cd819 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -36,6 +36,7 @@ jobs: uses: actions/checkout@v3 with: ref: ${{ fromJSON(needs.release-please.outputs.release-pr).headBranchName }} + token: ${{ secrets.NOIR_RELEASES_TOKEN }} - name: Setup toolchain uses: dtolnay/rust-toolchain@1.65.0 @@ -46,8 +47,8 @@ jobs: - name: Configure git run: | - git config user.name github-actions[bot] - git config user.email 41898282+github-actions[bot]@users.noreply.github.com + git config user.name kevaundray + git config user.email kevtheappdev@gmail.com - name: Commit updates run: | diff --git a/Cargo.lock b/Cargo.lock index 55c25dff231..d886da120ef 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -418,7 +418,7 @@ dependencies = [ [[package]] name = "barretenberg_wrapper" version = "0.1.0" -source = "git+https://github.com/noir-lang/aztec-connect?branch=kw/noir-dsl#f95b4d3551db4a62bbf7de56c6c3362523635e50" +source = "git+https://github.com/noir-lang/aztec-connect?branch=kw/noir-dsl#225bb52f9f9831fae94d0bfbd2f3c554b92ed039" dependencies = [ "bindgen", "cmake", @@ -2832,9 +2832,9 @@ dependencies = [ [[package]] name = "tokio-util" -version = "0.7.5" +version = "0.7.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7e267c18a719545b481171952a79f8c25c80361463ba44bc7fa9eba7c742ef4f" +checksum = "bc6a3b08b64e6dfad376fa2432c7b1f01522e37a623c3050bc95db2d3ff21583" dependencies = [ "bytes", "futures-core", @@ -3082,9 +3082,9 @@ dependencies = [ [[package]] name = "wasm-encoder" -version = "0.22.1" +version = "0.23.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9a584273ccc2d9311f1dd19dc3fb26054661fa3e373d53ede5d1144ba07a9acd" +checksum = "1c3e4bc09095436c8e7584d86d33e6c3ee67045af8fb262cbb9cc321de553428" dependencies = [ "leb128", ] @@ -3328,9 +3328,9 @@ checksum = "718ed7c55c2add6548cca3ddd6383d738cd73b892df400e96b9aa876f0141d7a" [[package]] name = "wast" -version = "52.0.3" +version = "53.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "15942180f265280eede7bc38b239e9770031d1821c02d905284216c645316430" +checksum = "8244fa24196b1d8fd3ca4a96a3a164c40f846498c5deab6caf414c67340ca4af" dependencies = [ "leb128", "memchr", @@ -3340,9 +3340,9 @@ dependencies = [ [[package]] name = "wat" -version = "1.0.57" +version = "1.0.58" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "37212100d4cbe6f0f6ff6e707f1e5a5b5b675f0451231ed9e4235e234e127ed3" +checksum = "4620f1059add6dad511decb9d5d88b4a0a0d3e2e315ed34f79b0dc0dce18aa4b" dependencies = [ "wast", ] diff --git a/crates/nargo/src/cli/mod.rs b/crates/nargo/src/cli/mod.rs index 92cc68b41d9..f78957c80d4 100644 --- a/crates/nargo/src/cli/mod.rs +++ b/crates/nargo/src/cli/mod.rs @@ -93,7 +93,12 @@ pub fn start_cli() { Arg::with_name("test_name") .help("If given, only tests with names containing this string will be run"), ) - .arg(allow_warnings.clone()), + .arg(allow_warnings.clone()) + .arg( + Arg::with_name("show-logs") + .long("show-logs") + .help("Display output of println statements during tests"), + ), ) .subcommand( App::new("compile") diff --git a/crates/nargo/src/cli/test_cmd.rs b/crates/nargo/src/cli/test_cmd.rs index 141bc034f40..b651f3d7b66 100644 --- a/crates/nargo/src/cli/test_cmd.rs +++ b/crates/nargo/src/cli/test_cmd.rs @@ -18,13 +18,19 @@ pub(crate) fn run(args: ArgMatches) -> Result<(), CliError> { let args = args.subcommand_matches("test").unwrap(); let test_name = args.value_of("test_name").unwrap_or(""); let allow_warnings = args.is_present("allow-warnings"); + let show_output = args.is_present("show-logs"); let program_dir = args.value_of("path").map_or_else(|| std::env::current_dir().unwrap(), PathBuf::from); - run_tests(&program_dir, test_name, allow_warnings) + run_tests(&program_dir, test_name, allow_warnings, show_output) } -fn run_tests(program_dir: &Path, test_name: &str, allow_warnings: bool) -> Result<(), CliError> { +fn run_tests( + program_dir: &Path, + test_name: &str, + allow_warnings: bool, + show_output: bool, +) -> Result<(), CliError> { let backend = crate::backends::ConcreteBackend; let mut driver = Resolver::resolve_root_config(program_dir, backend.np_language())?; @@ -43,10 +49,10 @@ fn run_tests(program_dir: &Path, test_name: &str, allow_warnings: bool) -> Resul for test_function in test_functions { let test_name = driver.function_name(test_function); - write!(writer, "Testing {test_name}... ").expect("Failed to write to stdout"); + writeln!(writer, "Testing {test_name}...").expect("Failed to write to stdout"); writer.flush().ok(); - match run_test(test_name, test_function, &driver, allow_warnings) { + match run_test(test_name, test_function, &driver, allow_warnings, show_output) { Ok(_) => { writer.set_color(ColorSpec::new().set_fg(Some(Color::Green))).ok(); writeln!(writer, "ok").ok(); @@ -76,12 +82,13 @@ fn run_test( main: FuncId, driver: &Driver, allow_warnings: bool, + show_output: bool, ) -> Result<(), CliError> { let backend = crate::backends::ConcreteBackend; let language = backend.np_language(); let program = driver - .compile_no_check(language, false, allow_warnings, Some(main)) + .compile_no_check(language, false, allow_warnings, Some(main), show_output) .map_err(|_| CliError::Generic(format!("Test '{test_name}' failed to compile")))?; let mut solved_witness = BTreeMap::new(); diff --git a/crates/nargo/tests/test_data/strings/Prover.toml b/crates/nargo/tests/test_data/strings/Prover.toml index 1e5f7c6f6e2..0d1bd92b5de 100644 --- a/crates/nargo/tests/test_data/strings/Prover.toml +++ b/crates/nargo/tests/test_data/strings/Prover.toml @@ -1,3 +1,4 @@ message = "hello world" +y = 5 hex_as_string = "0x41" -hex_as_field = "0x41" \ No newline at end of file +hex_as_field = "0x41" diff --git a/crates/nargo/tests/test_data/strings/src/main.nr b/crates/nargo/tests/test_data/strings/src/main.nr index 83574e9c068..a22ad06af0f 100644 --- a/crates/nargo/tests/test_data/strings/src/main.nr +++ b/crates/nargo/tests/test_data/strings/src/main.nr @@ -1,12 +1,56 @@ -fn main(message : pub str<11>, hex_as_string : str<4>, hex_as_field : Field) { +use dep::std; + +fn main(message : pub str<11>, y : Field, hex_as_string : str<4>, hex_as_field : Field) { let mut bad_message = "hello world"; constrain message == "hello world"; bad_message = "helld world"; + let x = 10; + let z = x * 5; + std::println(10); + + std::println(z); // x * 5 in println not yet supported + std::println(x); + + let array = [1, 2, 3, 5, 8]; + constrain y == 5; // Change to y != 5 to see how the later print statements are not called + std::println(array); + std::println(bad_message); constrain message != bad_message; + let hash = std::hash::pedersen([x]); + std::println(hash); + constrain hex_as_string == "0x41"; // constrain hex_as_string != 0x41; This will fail with a type mismatch between str[4] and Field constrain hex_as_field == 0x41; +} + +#[test] +fn test_prints_strings() { + let message = "hello world!"; + + std::println(message); + std::println("goodbye world"); +} + +#[test] +fn test_prints_array() { + let array = [1, 2, 3, 5, 8]; + + // TODO: Printing structs currently not supported + // let s = Test { a: 1, b: 2, c: [3, 4] }; + // std::println(s); + + std::println(array); + + let hash = std::hash::pedersen(array); + std::println(hash); +} + +struct Test { + a: Field, + b: Field, + c: [Field; 2], } \ No newline at end of file diff --git a/crates/noirc_abi/src/lib.rs b/crates/noirc_abi/src/lib.rs index 7d8bc645102..685f1bf5e16 100644 --- a/crates/noirc_abi/src/lib.rs +++ b/crates/noirc_abi/src/lib.rs @@ -4,8 +4,8 @@ use std::{collections::BTreeMap, convert::TryInto, str}; use acvm::FieldElement; use errors::AbiError; use input_parser::InputValue; +use iter_extended::vecmap; use serde::{Deserialize, Serialize}; - // This is the ABI used to bridge the different TOML formats for the initial // witness, the partial witness generator and the interpreter. // @@ -260,19 +260,10 @@ impl Abi { InputValue::Vec(field_elements) } AbiType::String { length } => { - let string_as_slice: Vec = field_iterator - .take(*length as usize) - .map(|e| { - let mut field_as_bytes = e.to_be_bytes(); - let char_byte = field_as_bytes.pop().unwrap(); // A character in a string is represented by a u8, thus we just want the last byte of the element - assert!(field_as_bytes.into_iter().all(|b| b == 0)); // Assert that the rest of the field element's bytes are empty - char_byte - }) - .collect(); - - let final_string = str::from_utf8(&string_as_slice).unwrap(); - - InputValue::String(final_string.to_owned()) + let field_elements: Vec = + field_iterator.take(*length as usize).collect(); + + InputValue::String(decode_string_value(&field_elements)) } AbiType::Struct { fields, .. } => { let mut struct_map = BTreeMap::new(); @@ -290,3 +281,15 @@ impl Abi { Ok(value) } } + +pub fn decode_string_value(field_elements: &[FieldElement]) -> String { + let string_as_slice = vecmap(field_elements, |e| { + let mut field_as_bytes = e.to_be_bytes(); + let char_byte = field_as_bytes.pop().unwrap(); // A character in a string is represented by a u8, thus we just want the last byte of the element + assert!(field_as_bytes.into_iter().all(|b| b == 0)); // Assert that the rest of the field element's bytes are empty + char_byte + }); + + let final_string = str::from_utf8(&string_as_slice).unwrap(); + final_string.to_owned() +} diff --git a/crates/noirc_driver/src/lib.rs b/crates/noirc_driver/src/lib.rs index 69d19517585..608f310381f 100644 --- a/crates/noirc_driver/src/lib.rs +++ b/crates/noirc_driver/src/lib.rs @@ -151,7 +151,7 @@ impl Driver { allow_warnings: bool, ) -> Result { self.check_crate(allow_warnings)?; - self.compile_no_check(np_language, show_ssa, allow_warnings, None) + self.compile_no_check(np_language, show_ssa, allow_warnings, None, true) } /// Compile the current crate. Assumes self.check_crate is called beforehand! @@ -163,6 +163,7 @@ impl Driver { allow_warnings: bool, // Optional override to provide a different `main` function to start execution main_function: Option, + show_output: bool, ) -> Result { // Find the local crate, one should always be present let local_crate = self.context.def_map(LOCAL_CRATE).unwrap(); @@ -187,7 +188,7 @@ impl Driver { let program = monomorphize(main_function, &self.context.def_interner); let blackbox_supported = acvm::default_is_black_box_supported(np_language.clone()); - match create_circuit(program, np_language, blackbox_supported, show_ssa) { + match create_circuit(program, np_language, blackbox_supported, show_ssa, show_output) { Ok(circuit) => Ok(CompiledProgram { circuit, abi: Some(abi) }), Err(err) => { // The FileId here will be the file id of the file with the main file diff --git a/crates/noirc_evaluator/src/lib.rs b/crates/noirc_evaluator/src/lib.rs index a4fea3e0acc..ebcf74680c9 100644 --- a/crates/noirc_evaluator/src/lib.rs +++ b/crates/noirc_evaluator/src/lib.rs @@ -39,11 +39,12 @@ pub fn create_circuit( np_language: Language, is_blackbox_supported: IsBlackBoxSupported, enable_logging: bool, + show_output: bool, ) -> Result { let mut evaluator = Evaluator::new(); // First evaluate the main function - evaluator.evaluate_main_alt(program, enable_logging)?; + evaluator.evaluate_main_alt(program, enable_logging, show_output)?; let witness_index = evaluator.current_witness_index(); @@ -110,6 +111,7 @@ impl Evaluator { &mut self, program: Program, enable_logging: bool, + show_output: bool, ) -> Result<(), RuntimeError> { let mut ir_gen = IrGenerator::new(program); self.parse_abi_alt(&mut ir_gen); @@ -118,7 +120,7 @@ impl Evaluator { ir_gen.ssa_gen_main()?; //Generates ACIR representation: - ir_gen.context.ir_to_acir(self, enable_logging)?; + ir_gen.context.ir_to_acir(self, enable_logging, show_output)?; Ok(()) } diff --git a/crates/noirc_evaluator/src/ssa/acir_gen.rs b/crates/noirc_evaluator/src/ssa/acir_gen.rs index 6ac54ef7afa..0eab410d733 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen.rs @@ -1,4 +1,5 @@ use crate::ssa::{ + builtin, context::SsaContext, node::{Instruction, Operation}, }; @@ -34,6 +35,7 @@ impl Acir { ins: &Instruction, evaluator: &mut Evaluator, ctx: &SsaContext, + show_output: bool, ) -> Result<(), RuntimeErrorKind> { use operations::{ binary, condition, constrain, intrinsics, load, not, r#return, store, truncate, @@ -57,7 +59,16 @@ impl Acir { truncate::evaluate(value, *bit_size, *max_bit_size, var_cache, evaluator, ctx) } Operation::Intrinsic(opcode, args) => { - intrinsics::evaluate(args, ins, *opcode, var_cache, acir_mem, ctx, evaluator) + let opcode = match opcode { + builtin::Opcode::Println(print_info) => { + builtin::Opcode::Println(builtin::PrintlnInfo { + is_string_output: print_info.is_string_output, + show_output, + }) + } + _ => *opcode, + }; + intrinsics::evaluate(args, ins, opcode, var_cache, acir_mem, ctx, evaluator) } Operation::Return(node_ids) => { r#return::evaluate(node_ids, acir_mem, var_cache, evaluator, ctx)? diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/internal_var.rs b/crates/noirc_evaluator/src/ssa/acir_gen/internal_var.rs index a98888a9062..c614d940d50 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/internal_var.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/internal_var.rs @@ -45,6 +45,9 @@ impl InternalVar { pub(crate) fn set_id(&mut self, id: NodeId) { self.id = Some(id) } + pub(crate) fn cached_witness(&self) -> &Option { + &self.cached_witness + } pub fn to_expression(&self) -> Expression { if let Some(w) = self.cached_witness { diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/operations/intrinsics.rs b/crates/noirc_evaluator/src/ssa/acir_gen/operations/intrinsics.rs index 942a00f0137..caeb994c8d0 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/operations/intrinsics.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/operations/intrinsics.rs @@ -13,12 +13,15 @@ use crate::{ }, Evaluator, }; -use acvm::acir::{ - circuit::{ - directives::Directive, - opcodes::{BlackBoxFuncCall, FunctionInput, Opcode as AcirOpcode}, +use acvm::{ + acir::{ + circuit::{ + directives::{Directive, LogInfo}, + opcodes::{BlackBoxFuncCall, FunctionInput, Opcode as AcirOpcode}, + }, + native_types::{Expression, Witness}, }, - native_types::{Expression, Witness}, + FieldElement, }; use iter_extended::vecmap; @@ -62,6 +65,19 @@ pub(crate) fn evaluate( memory_map.map_array(a, &outputs, ctx); } } + Opcode::Println(print_info) => { + outputs = Vec::new(); // print statements do not output anything + if print_info.show_output { + evaluate_println( + var_cache, + memory_map, + print_info.is_string_output, + args, + ctx, + evaluator, + ); + } + } Opcode::LowLevel(op) => { let inputs = prepare_inputs(var_cache, memory_map, args, ctx, evaluator); let output_count = op.definition().output_size.0 as u32; @@ -115,7 +131,7 @@ pub(crate) fn evaluate( // If more than witness is returned, // the result is inside the result type of `Instruction` // as a pointer to an array - (outputs.len() == 1).then_some(Expression::from(&outputs[0])).map(InternalVar::from) + (outputs.len() == 1).then(|| InternalVar::from(outputs[0])) } // Transform the arguments of intrinsic functions into witnesses @@ -221,3 +237,95 @@ fn prepare_outputs( } outputs } + +fn evaluate_println( + var_cache: &mut InternalVarCache, + memory_map: &mut AcirMem, + is_string_output: bool, + args: &[NodeId], + ctx: &SsaContext, + evaluator: &mut Evaluator, +) { + assert_eq!(args.len(), 1, "print statements can only support one argument"); + let node_id = args[0]; + + let mut log_string = "".to_owned(); + let mut log_witnesses = Vec::new(); + + let obj_type = ctx.object_type(node_id); + match obj_type { + ObjectType::Pointer(array_id) => { + let mem_array = &ctx.mem[array_id]; + let mut field_elements = Vec::new(); + for idx in 0..mem_array.len { + let var = memory_map + .load_array_element_constant_index(mem_array, idx) + .expect("array element being logged does not exist in memory"); + let array_elem_expr = var.expression(); + if array_elem_expr.is_const() { + field_elements.push(array_elem_expr.q_c); + } else { + let var = match var_cache.get(&node_id) { + Some(var) => var.clone(), + _ => InternalVar::from(array_elem_expr.clone()), + }; + let w = var + .cached_witness() + .expect("array element to be logged is missing a witness"); + log_witnesses.push(w); + } + } + + if is_string_output { + let final_string = noirc_abi::decode_string_value(&field_elements); + log_string.push_str(&final_string); + } else if !field_elements.is_empty() { + let fields = vecmap(field_elements, format_field_string); + log_string = format!("[{}]", fields.join(", ")); + } + } + _ => match ctx.get_as_constant(node_id) { + Some(field) => { + log_string = format_field_string(field); + } + None => { + let var = var_cache.get(&node_id).unwrap_or_else(|| { + panic!( + "invalid input for print statement: {:?}", + ctx.try_get_node(node_id).expect("node is missing from SSA") + ) + }); + if let Some(field) = var.to_const() { + log_string = format_field_string(field); + } else if let Some(w) = var.cached_witness() { + log_witnesses.push(*w); + } else { + unreachable!("array element to be logged is missing a witness"); + } + } + }, + }; + + // Only one of the witness vector or the output string should be non-empty + assert!(log_witnesses.is_empty() ^ log_string.is_empty()); + + let log_directive = if !log_string.is_empty() { + Directive::Log(LogInfo::FinalizedOutput(log_string)) + } else { + Directive::Log(LogInfo::WitnessOutput(log_witnesses)) + }; + + evaluator.opcodes.push(AcirOpcode::Directive(log_directive)); +} + +/// This trims any leading zeroes. +/// A singular '0' will be prepended as well if the trimmed string has an odd length. +/// A hex string's length needs to be even to decode into bytes, as two digits correspond to +/// one byte. +fn format_field_string(field: FieldElement) -> String { + let mut trimmed_field = field.to_hex().trim_start_matches('0').to_owned(); + if trimmed_field.len() % 2 != 0 { + trimmed_field = "0".to_owned() + &trimmed_field + }; + "0x".to_owned() + &trimmed_field +} diff --git a/crates/noirc_evaluator/src/ssa/builtin.rs b/crates/noirc_evaluator/src/ssa/builtin.rs index b6bcf21a8b4..7c86b1a0c93 100644 --- a/crates/noirc_evaluator/src/ssa/builtin.rs +++ b/crates/noirc_evaluator/src/ssa/builtin.rs @@ -15,6 +15,7 @@ pub enum Opcode { LowLevel(BlackBoxFunc), ToBits, ToRadix, + Println(PrintlnInfo), Sort, } @@ -34,6 +35,9 @@ impl Opcode { match op_name { "to_le_bits" => Some(Opcode::ToBits), "to_radix" => Some(Opcode::ToRadix), + "println" => { + Some(Opcode::Println(PrintlnInfo { is_string_output: false, show_output: true })) + } "arraysort" => Some(Opcode::Sort), _ => BlackBoxFunc::lookup(op_name).map(Opcode::LowLevel), } @@ -44,6 +48,7 @@ impl Opcode { Opcode::LowLevel(op) => op.name(), Opcode::ToBits => "to_le_bits", Opcode::ToRadix => "to_radix", + Opcode::Println(_) => "println", Opcode::Sort => "arraysort", } } @@ -70,7 +75,7 @@ impl Opcode { } } } - Opcode::ToBits | Opcode::ToRadix | Opcode::Sort => BigUint::zero(), //pointers do not overflow + Opcode::ToBits | Opcode::ToRadix | Opcode::Println(_) | Opcode::Sort => BigUint::zero(), //pointers do not overflow } } @@ -98,6 +103,7 @@ impl Opcode { } Opcode::ToBits => (FieldElement::max_num_bits(), ObjectType::boolean()), Opcode::ToRadix => (FieldElement::max_num_bits(), ObjectType::native_field()), + Opcode::Println(_) => (0, ObjectType::NotAnObject), Opcode::Sort => { let a = super::mem::Memory::deref(ctx, args[0]).unwrap(); (ctx.mem[a].len, ctx.mem[a].element_type) @@ -105,3 +111,12 @@ impl Opcode { } } } + +#[derive(Clone, Debug, Hash, Copy, PartialEq, Eq)] +pub struct PrintlnInfo { + // We store strings as arrays and there is no differentiation between them in the SSA. + // This bool simply states whether an array that is to be printed should be outputted as a utf8 string + pub is_string_output: bool, + // This is a flag used during `nargo test` to determine whether to display println output. + pub show_output: bool, +} diff --git a/crates/noirc_evaluator/src/ssa/context.rs b/crates/noirc_evaluator/src/ssa/context.rs index db9304e9099..8706f36d1ac 100644 --- a/crates/noirc_evaluator/src/ssa/context.rs +++ b/crates/noirc_evaluator/src/ssa/context.rs @@ -14,7 +14,7 @@ use crate::ssa::{ use crate::Evaluator; use acvm::FieldElement; use iter_extended::vecmap; -use noirc_frontend::monomorphization::ast::{Definition, FuncId}; +use noirc_frontend::monomorphization::ast::{Definition, Expression, FuncId, Literal, Type}; use num_bigint::BigUint; use num_traits::{One, Zero}; use std::collections::{HashMap, HashSet}; @@ -667,6 +667,7 @@ impl SsaContext { &mut self, evaluator: &mut Evaluator, enable_logging: bool, + show_output: bool, ) -> Result<(), RuntimeError> { //SSA self.log(enable_logging, "SSA:", "\ninline functions"); @@ -712,7 +713,7 @@ impl SsaContext { integer::overflow_strategy(self)?; self.log(enable_logging, "\noverflow:", ""); //ACIR - self.acir(evaluator)?; + self.acir(evaluator, show_output)?; if enable_logging { print_acir_circuit(&evaluator.opcodes); println!("DONE"); @@ -721,13 +722,13 @@ impl SsaContext { Ok(()) } - pub fn acir(&self, evaluator: &mut Evaluator) -> Result<(), RuntimeError> { + pub fn acir(&self, evaluator: &mut Evaluator, show_output: bool) -> Result<(), RuntimeError> { let mut acir = Acir::default(); let mut fb = Some(&self[self.first_block]); while let Some(block) = fb { for iter in &block.instructions { let ins = self.instruction(*iter); - acir.acir_gen_instruction(ins, evaluator, self).map_err(RuntimeError::from)?; + acir.acir_gen_instruction(ins, evaluator, self, show_output)?; } //TODO we should rather follow the jumps fb = block.left.map(|block_id| &self[block_id]); @@ -1105,15 +1106,46 @@ impl SsaContext { NodeId(index) } - pub fn get_builtin_opcode(&self, node_id: NodeId) -> Option { + pub fn get_builtin_opcode( + &self, + node_id: NodeId, + arguments: &[Expression], + ) -> Option { match &self[node_id] { - NodeObject::Function(FunctionKind::Builtin(opcode), ..) => Some(*opcode), + NodeObject::Function(FunctionKind::Builtin(opcode), ..) => match opcode { + builtin::Opcode::Println(_) => { + // Compiler sanity check. This should be caught during typechecking + assert_eq!( + arguments.len(), + 1, + "print statements currently only support one argument" + ); + let is_string = match &arguments[0] { + Expression::Ident(ident) => match ident.typ { + Type::String(_) => true, + Type::Tuple(_) => { + unreachable!("logging structs/tuples is not supported") + } + Type::Function { .. } => { + unreachable!("logging functions is not supported") + } + _ => false, + }, + Expression::Literal(literal) => matches!(literal, Literal::Str(_)), + _ => unreachable!("logging this expression type is not supported"), + }; + Some(builtin::Opcode::Println(builtin::PrintlnInfo { + is_string_output: is_string, + show_output: true, + })) + } + _ => Some(*opcode), + }, _ => None, } } - pub fn convert_type(&mut self, t: &noirc_frontend::monomorphization::ast::Type) -> ObjectType { - use noirc_frontend::monomorphization::ast::Type; + pub fn convert_type(&mut self, t: &Type) -> ObjectType { use noirc_frontend::Signedness; match t { Type::Bool => ObjectType::boolean(), diff --git a/crates/noirc_evaluator/src/ssa/function.rs b/crates/noirc_evaluator/src/ssa/function.rs index 34f98c7a886..e7ef6aadb67 100644 --- a/crates/noirc_evaluator/src/ssa/function.rs +++ b/crates/noirc_evaluator/src/ssa/function.rs @@ -230,7 +230,7 @@ impl IrGenerator { let func = self.ssa_gen_expression(&call.func)?.unwrap_id(); let arguments = self.ssa_gen_expression_list(&call.arguments); - if let Some(opcode) = self.context.get_builtin_opcode(func) { + if let Some(opcode) = self.context.get_builtin_opcode(func, &call.arguments) { return self.call_low_level(opcode, arguments); } diff --git a/crates/noirc_evaluator/src/ssa/optimizations.rs b/crates/noirc_evaluator/src/ssa/optimizations.rs index d9f2b042f78..65e9e4a8a66 100644 --- a/crates/noirc_evaluator/src/ssa/optimizations.rs +++ b/crates/noirc_evaluator/src/ssa/optimizations.rs @@ -327,9 +327,14 @@ fn cse_block_with_anchor( new_list.push(*ins_id); } Operation::Return(..) => new_list.push(*ins_id), - Operation::Intrinsic(_, args) => { + Operation::Intrinsic(opcode, args) => { //Add dummy load for function arguments and enable CSE only if no array in argument let mut activate_cse = true; + // We do not want to replace any print intrinsics as we want them to remain in order and unchanged + if let builtin::Opcode::Println(_) = opcode { + activate_cse = false + } + for arg in args { if let Some(obj) = ctx.try_get_node(*arg) { if let ObjectType::Pointer(a) = obj.get_type() { @@ -381,19 +386,25 @@ fn cse_block_with_anchor( //cannot simplify to_le_bits() in the previous call because it get replaced with multiple instructions if let Operation::Intrinsic(opcode, args) = &update2.operation { - let args = args.iter().map(|arg| { - NodeEval::from_id(ctx, *arg).into_const_value().map(|f| f.to_u128()) - }); - - if let Some(args) = args.collect() { - update2.mark = Mark::Deleted; - new_list.extend(evaluate_intrinsic( - ctx, - *opcode, - args, - &update2.res_type, - block_id, - )); + match opcode { + // We do not simplify print statements + builtin::Opcode::Println(_) => (), + _ => { + let args = args.iter().map(|arg| { + NodeEval::from_id(ctx, *arg).into_const_value().map(|f| f.to_u128()) + }); + + if let Some(args) = args.collect() { + update2.mark = Mark::Deleted; + new_list.extend(evaluate_intrinsic( + ctx, + *opcode, + args, + &update2.res_type, + block_id, + )); + } + } } } let update3 = ctx.instruction_mut(*ins_id); diff --git a/crates/noirc_frontend/src/ast/statement.rs b/crates/noirc_frontend/src/ast/statement.rs index 7c69147c0b0..279b3054c7c 100644 --- a/crates/noirc_frontend/src/ast/statement.rs +++ b/crates/noirc_frontend/src/ast/statement.rs @@ -128,7 +128,6 @@ pub enum Statement { // This is an expression with a trailing semi-colon // terminology Taken from rustc Semi(Expression), - // This statement is the result of a recovered parse error. // To avoid issuing multiple errors in later steps, it should // be skipped in any future analysis if possible. diff --git a/noir_stdlib/src/lib.nr b/noir_stdlib/src/lib.nr index 3435dfc0f5a..dc87dab3d75 100644 --- a/noir_stdlib/src/lib.nr +++ b/noir_stdlib/src/lib.nr @@ -8,6 +8,9 @@ mod sha256; mod sha512; mod field; +#[builtin(println)] +fn println(_input : T) {} + // Returns base^exponent. // ^ means to the power of and not xor // Caution: we assume the exponent fits into 32 bits