From 776b486e076fa1abd01c8b2cb6ebcf8888d0577f Mon Sep 17 00:00:00 2001 From: Tom French Date: Wed, 3 Jan 2024 08:51:41 +0000 Subject: [PATCH 01/13] feat: decompose `Instruction::Cast` to have an explicit truncation --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 41 ++----------------- .../src/ssa/function_builder/mod.rs | 19 +++++++++ 2 files changed, 22 insertions(+), 38 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 33f00796c9d..24a67ca58a1 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -469,9 +469,9 @@ impl Context { self.acir_context.assert_eq_var(lhs, rhs, assert_message.clone())?; } } - Instruction::Cast(value_id, typ) => { - let result_acir_var = self.convert_ssa_cast(value_id, typ, dfg)?; - self.define_result_var(dfg, instruction_id, result_acir_var); + Instruction::Cast(value_id, _) => { + let acir_var = self.convert_numeric_value(*value_id, dfg)?; + self.define_result_var(dfg, instruction_id, acir_var); } Instruction::Call { func, arguments } => { let result_ids = dfg.instruction_results(instruction_id); @@ -1633,41 +1633,6 @@ impl Context { } } - /// Returns an `AcirVar` that is constrained to fit in the target type by truncating the input. - /// If the target cast is to a `NativeField`, no truncation is required so the cast becomes a - /// no-op. - fn convert_ssa_cast( - &mut self, - value_id: &ValueId, - typ: &Type, - dfg: &DataFlowGraph, - ) -> Result { - let (variable, incoming_type) = match self.convert_value(*value_id, dfg) { - AcirValue::Var(variable, typ) => (variable, typ), - AcirValue::DynamicArray(_) | AcirValue::Array(_) => { - unreachable!("Cast is only applied to numerics") - } - }; - let target_numeric = match typ { - Type::Numeric(numeric) => numeric, - _ => unreachable!("Can only cast to a numeric"), - }; - match target_numeric { - NumericType::NativeField => { - // Casting into a Field as a no-op - Ok(variable) - } - NumericType::Unsigned { bit_size } | NumericType::Signed { bit_size } => { - let max_bit_size = incoming_type.bit_size(); - if max_bit_size <= *bit_size { - // Incoming variable already fits into target bit size - this is a no-op - return Ok(variable); - } - self.acir_context.truncate_var(variable, *bit_size, max_bit_size) - } - } - } - /// Returns an `AcirVar`that is constrained to be result of the truncation. fn convert_ssa_truncate( &mut self, diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index b972afa2990..03ca4b07751 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -230,6 +230,25 @@ impl FunctionBuilder { /// Insert a cast instruction at the end of the current block. /// Returns the result of the cast instruction. pub(crate) fn insert_cast(&mut self, value: ValueId, typ: Type) -> ValueId { + fn get_type_size(typ: &Type) -> u32 { + match typ { + Type::Numeric(NumericType::NativeField) => FieldElement::max_num_bits(), + Type::Numeric( + NumericType::Unsigned { bit_size } | NumericType::Signed { bit_size }, + ) => *bit_size, + _ => unreachable!("Should only be called with primitive types"), + } + } + + let incoming_type = self.type_of_value(value); + let incoming_type_size = get_type_size(&incoming_type); + let target_type_size = get_type_size(&typ); + let value = if target_type_size < incoming_type_size { + self.insert_truncate(value, target_type_size, incoming_type_size) + } else { + value + }; + self.insert_instruction(Instruction::Cast(value, typ), None).first() } From 60866a8d849f420b185d427f9198527ca37e5665 Mon Sep 17 00:00:00 2001 From: Tom French Date: Thu, 4 Jan 2024 12:20:29 +0000 Subject: [PATCH 02/13] chore: fix `FromField` --- .../src/ssa/ir/instruction/call.rs | 32 +++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index b07e2df7bd3..c9f8ab98b6c 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -1,6 +1,7 @@ use std::{collections::VecDeque, rc::Rc}; use acvm::{acir::BlackBoxFunc, BlackBoxResolutionError, FieldElement}; +use im::Vector; use iter_extended::vecmap; use num_bigint::BigUint; @@ -10,7 +11,7 @@ use crate::ssa::{ dfg::{CallStack, DataFlowGraph}, instruction::Intrinsic, map::Id, - types::Type, + types::{NumericType, Type}, value::{Value, ValueId}, }, opt::flatten_cfg::value_merger::ValueMerger, @@ -242,7 +243,34 @@ pub(super) fn simplify_call( SimplifyResult::SimplifiedToInstruction(instruction) } Intrinsic::FromField => { - let instruction = Instruction::Cast(arguments[0], ctrl_typevars.unwrap().remove(0)); + let incoming_type = Type::field(); + let target_type = ctrl_typevars.unwrap().remove(0); + + fn get_type_size(typ: &Type) -> u32 { + match typ { + Type::Numeric(NumericType::NativeField) => FieldElement::max_num_bits(), + Type::Numeric( + NumericType::Unsigned { bit_size } | NumericType::Signed { bit_size }, + ) => *bit_size, + _ => unreachable!("Should only be called with primitive types"), + } + } + + let truncate = Instruction::Truncate { + value: arguments[0], + bit_size: get_type_size(&target_type), + max_bit_size: get_type_size(&incoming_type), + }; + let truncated_value = dfg + .insert_instruction_and_results( + truncate, + block, + Some(vec![incoming_type]), + Vector::default(), // needs a callstack + ) + .first(); + + let instruction = Instruction::Cast(truncated_value, target_type); SimplifyResult::SimplifiedToInstruction(instruction) } } From 1e632f4b4ca2286b3593529bbe9ef6cee195dd41 Mon Sep 17 00:00:00 2001 From: Tom French Date: Thu, 4 Jan 2024 12:27:23 +0000 Subject: [PATCH 03/13] chore: make truncation a pure instruction --- compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 628ad638e64..6ab954f81b6 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -236,10 +236,7 @@ impl Instruction { // In ACIR, a division with a false predicate outputs (0,0), so it cannot replace another instruction unless they have the same predicate bin.operator != BinaryOp::Div } - Cast(_, _) | Not(_) | ArrayGet { .. } | ArraySet { .. } => true, - - // Unclear why this instruction causes problems. - Truncate { .. } => false, + Cast(_, _) | Truncate { .. } | Not(_) | ArrayGet { .. } | ArraySet { .. } => true, // These either have side-effects or interact with memory Constrain(..) From 9966f8e7d0ddb839c303fe8986bd42b209a3b7d5 Mon Sep 17 00:00:00 2001 From: Tom French Date: Thu, 4 Jan 2024 13:04:13 +0000 Subject: [PATCH 04/13] chore: update test --- .../src/ssa/opt/constant_folding.rs | 29 ++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index e944d7d99d8..4d2e1290007 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -294,16 +294,18 @@ mod test { fn instruction_deduplication() { // fn main f0 { // b0(v0: Field): - // v1 = cast v0 as u32 - // v2 = cast v0 as u32 - // constrain v1 v2 + // v1 = truncate v0 to 32 bits, max_bit_size: 254 + // v2 = cast v1 as u32 + // v3 = truncate v0 to 32 bits, max_bit_size: 254 + // v4 = cast v3 as u32 + // constrain v2 v4 // } // - // After constructing this IR, we run constant folding which should replace the second cast + // After constructing this IR, we run constant folding which should replace the second truncation and cast // with a reference to the results to the first. This then allows us to optimize away // the constrain instruction as both inputs are known to be equal. // - // The first cast instruction is retained and will be removed in the dead instruction elimination pass. + // The first truncation and cast instructions are retained and will be removed in the dead instruction elimination pass. let main_id = Id::test_new(0); // Compiling main @@ -317,21 +319,28 @@ mod test { let mut ssa = builder.finish(); let main = ssa.main_mut(); let instructions = main.dfg[main.entry_block()].instructions(); - assert_eq!(instructions.len(), 3); + assert_eq!(instructions.len(), 5); // Expected output: // // fn main f0 { // b0(v0: Field): - // v1 = cast v0 as u32 + // v1 = truncate v0 to 32 bits, max_bit_size: 254 + // v2 = cast v1 as u32 // } let ssa = ssa.fold_constants(); let main = ssa.main(); let instructions = main.dfg[main.entry_block()].instructions(); - assert_eq!(instructions.len(), 1); - let instruction = &main.dfg[instructions[0]]; + assert_eq!(instructions.len(), 2); - assert_eq!(instruction, &Instruction::Cast(ValueId::test_new(0), Type::unsigned(32))); + assert_eq!( + &main.dfg[instructions[0]], + &Instruction::Truncate { value: v0, bit_size: 32, max_bit_size: 254 } + ); + assert_eq!( + &main.dfg[instructions[1]], + &Instruction::Cast(ValueId::test_new(5), Type::unsigned(32)) + ); } } From 1875f1e788de5b53e2edb1d79a92f9aa1fe8eba2 Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 5 Jan 2024 08:58:37 +0000 Subject: [PATCH 05/13] chore: move `get_type_size` into `bit_wise` method on `Type` --- .../src/ssa/function_builder/mod.rs | 15 +----- .../src/ssa/ir/instruction/call.rs | 16 ++----- compiler/noirc_evaluator/src/ssa/ir/types.rs | 47 +++++++++++++------ 3 files changed, 38 insertions(+), 40 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 03ca4b07751..14a8f3da917 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -230,19 +230,8 @@ impl FunctionBuilder { /// Insert a cast instruction at the end of the current block. /// Returns the result of the cast instruction. pub(crate) fn insert_cast(&mut self, value: ValueId, typ: Type) -> ValueId { - fn get_type_size(typ: &Type) -> u32 { - match typ { - Type::Numeric(NumericType::NativeField) => FieldElement::max_num_bits(), - Type::Numeric( - NumericType::Unsigned { bit_size } | NumericType::Signed { bit_size }, - ) => *bit_size, - _ => unreachable!("Should only be called with primitive types"), - } - } - - let incoming_type = self.type_of_value(value); - let incoming_type_size = get_type_size(&incoming_type); - let target_type_size = get_type_size(&typ); + let incoming_type_size = self.type_of_value(value).bit_size(); + let target_type_size = typ.bit_size(); let value = if target_type_size < incoming_type_size { self.insert_truncate(value, target_type_size, incoming_type_size) } else { diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index c9f8ab98b6c..5b3e4a3bee5 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -11,7 +11,7 @@ use crate::ssa::{ dfg::{CallStack, DataFlowGraph}, instruction::Intrinsic, map::Id, - types::{NumericType, Type}, + types::Type, value::{Value, ValueId}, }, opt::flatten_cfg::value_merger::ValueMerger, @@ -246,20 +246,10 @@ pub(super) fn simplify_call( let incoming_type = Type::field(); let target_type = ctrl_typevars.unwrap().remove(0); - fn get_type_size(typ: &Type) -> u32 { - match typ { - Type::Numeric(NumericType::NativeField) => FieldElement::max_num_bits(), - Type::Numeric( - NumericType::Unsigned { bit_size } | NumericType::Signed { bit_size }, - ) => *bit_size, - _ => unreachable!("Should only be called with primitive types"), - } - } - let truncate = Instruction::Truncate { value: arguments[0], - bit_size: get_type_size(&target_type), - max_bit_size: get_type_size(&incoming_type), + bit_size: target_type.bit_size(), + max_bit_size: incoming_type.bit_size(), }; let truncated_value = dfg .insert_instruction_and_results( diff --git a/compiler/noirc_evaluator/src/ssa/ir/types.rs b/compiler/noirc_evaluator/src/ssa/ir/types.rs index bae06a805d0..fa46ac40959 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/types.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/types.rs @@ -18,6 +18,27 @@ pub enum NumericType { NativeField, } +impl NumericType { + pub(crate) fn bit_size(self: &NumericType) -> u32 { + match self { + NumericType::NativeField => FieldElement::max_num_bits(), + NumericType::Unsigned { bit_size } | NumericType::Signed { bit_size } => *bit_size, + } + } + + /// Returns true if the given Field value is within the numeric limits + /// for the current NumericType. + pub(crate) fn value_is_within_limits(self, field: FieldElement) -> bool { + match self { + NumericType::Signed { bit_size } | NumericType::Unsigned { bit_size } => { + let max = 2u128.pow(bit_size) - 1; + field <= max.into() + } + NumericType::NativeField => true, + } + } +} + /// All types representable in the IR. #[derive(Clone, Debug, PartialEq, Eq, Hash, Ord, PartialOrd)] pub(crate) enum Type { @@ -68,6 +89,18 @@ impl Type { Type::Numeric(NumericType::NativeField) } + /// Returns the bit size of the provided numeric type. + /// + /// # Panics + /// + /// Panics if `self` is not a [`Type::Numeric`] + pub(crate) fn bit_size(&self) -> u32 { + match self { + Type::Numeric(numeric_type) => numeric_type.bit_size(), + other => panic!("bit_size: Expected numeric type, found {other}"), + } + } + /// Returns the size of the element type for this array/slice. /// The size of a type is defined as representing how many Fields are needed /// to represent the type. This is 1 for every primitive type, and is the number of fields @@ -122,20 +155,6 @@ impl Type { } } -impl NumericType { - /// Returns true if the given Field value is within the numeric limits - /// for the current NumericType. - pub(crate) fn value_is_within_limits(self, field: FieldElement) -> bool { - match self { - NumericType::Signed { bit_size } | NumericType::Unsigned { bit_size } => { - let max = 2u128.pow(bit_size) - 1; - field <= max.into() - } - NumericType::NativeField => true, - } - } -} - /// Composite Types are essentially flattened struct or tuple types. /// Array types may have these as elements where each flattened field is /// included in the array sequentially. From da1fb4d8ffcb55480f71e9dbfe2a85e498abfbe2 Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 5 Jan 2024 09:10:30 +0000 Subject: [PATCH 06/13] chore: create `insert_safe_cast` method on `FunctionContext` --- .../src/ssa/function_builder/mod.rs | 8 ------ .../src/ssa/ssa_gen/context.rs | 25 +++++++++++++++++++ .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 4 +-- 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 14a8f3da917..b972afa2990 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -230,14 +230,6 @@ impl FunctionBuilder { /// Insert a cast instruction at the end of the current block. /// Returns the result of the cast instruction. pub(crate) fn insert_cast(&mut self, value: ValueId, typ: Type) -> ValueId { - let incoming_type_size = self.type_of_value(value).bit_size(); - let target_type_size = typ.bit_size(); - let value = if target_type_size < incoming_type_size { - self.insert_truncate(value, target_type_size, incoming_type_size) - } else { - value - }; - self.insert_instruction(Instruction::Cast(value, typ), None).first() } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 5724bf56e8e..02c4f8e8413 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -665,6 +665,31 @@ impl<'a> FunctionContext<'a> { reshaped_return_values } + /// Inserts a cast instruction at the end of the current block and returns the results + /// of the cast. + /// + /// Compared to `self.builder.insert_cast`, this version will automatically truncate `value` to be a valid `typ`. + pub(super) fn insert_safe_cast( + &mut self, + value: ValueId, + typ: Type, + location: Location, + ) -> Result { + self.builder.set_location(location); + + // To ensure that `value` is a valid `typ`, we insert an `Instruction::Truncate` instruction beforehand if + // we're narrowing the type size. + let incoming_type_size = self.builder.type_of_value(value).bit_size(); + let target_type_size = typ.bit_size(); + let truncated_value = if target_type_size < incoming_type_size { + self.builder.insert_truncate(value, target_type_size, incoming_type_size) + } else { + value + }; + + Ok(self.builder.insert_cast(truncated_value, typ).into()) + } + /// Create a const offset of an address for an array load or store pub(super) fn make_offset(&mut self, mut address: ValueId, offset: u128) -> ValueId { if offset != 0 { diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index c00fbbbcb40..2358ccdc776 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -434,8 +434,8 @@ impl<'a> FunctionContext<'a> { fn codegen_cast(&mut self, cast: &ast::Cast) -> Result { let lhs = self.codegen_non_tuple_expression(&cast.lhs)?; let typ = Self::convert_non_tuple_type(&cast.r#type); - self.builder.set_location(cast.location); - Ok(self.builder.insert_cast(lhs, typ).into()) + + self.insert_safe_cast(lhs, typ, cast.location) } /// Codegens a for loop, creating three new blocks in the process. From 5941a6294e36bf529ad09f5512ae8af2d969d3fb Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 5 Jan 2024 09:50:29 +0000 Subject: [PATCH 07/13] chore: revert changes to `instruction_deduplication` test --- .../src/ssa/opt/constant_folding.rs | 37 +++++++------------ 1 file changed, 14 insertions(+), 23 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 4d2e1290007..1b89647bad2 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -187,7 +187,7 @@ mod test { instruction::{BinaryOp, Instruction, TerminatorInstruction}, map::Id, types::Type, - value::{Value, ValueId}, + value::Value, }, }; @@ -293,24 +293,22 @@ mod test { #[test] fn instruction_deduplication() { // fn main f0 { - // b0(v0: Field): - // v1 = truncate v0 to 32 bits, max_bit_size: 254 - // v2 = cast v1 as u32 - // v3 = truncate v0 to 32 bits, max_bit_size: 254 - // v4 = cast v3 as u32 - // constrain v2 v4 + // b0(v0: u16): + // v1 = cast v0 as u32 + // v2 = cast v0 as u32 + // constrain v1 v2 // } // - // After constructing this IR, we run constant folding which should replace the second truncation and cast + // After constructing this IR, we run constant folding which should replace the second cast // with a reference to the results to the first. This then allows us to optimize away // the constrain instruction as both inputs are known to be equal. // - // The first truncation and cast instructions are retained and will be removed in the dead instruction elimination pass. + // The first cast instruction is retained and will be removed in the dead instruction elimination pass. let main_id = Id::test_new(0); // Compiling main let mut builder = FunctionBuilder::new("main".into(), main_id, RuntimeType::Acir); - let v0 = builder.add_parameter(Type::field()); + let v0 = builder.add_parameter(Type::unsigned(16)); let v1 = builder.insert_cast(v0, Type::unsigned(32)); let v2 = builder.insert_cast(v0, Type::unsigned(32)); @@ -319,28 +317,21 @@ mod test { let mut ssa = builder.finish(); let main = ssa.main_mut(); let instructions = main.dfg[main.entry_block()].instructions(); - assert_eq!(instructions.len(), 5); + assert_eq!(instructions.len(), 3); // Expected output: // // fn main f0 { - // b0(v0: Field): - // v1 = truncate v0 to 32 bits, max_bit_size: 254 - // v2 = cast v1 as u32 + // b0(v0: u16): + // v1 = cast v1 as u32 // } let ssa = ssa.fold_constants(); let main = ssa.main(); let instructions = main.dfg[main.entry_block()].instructions(); - assert_eq!(instructions.len(), 2); + assert_eq!(instructions.len(), 1); + let instruction = &main.dfg[instructions[0]]; - assert_eq!( - &main.dfg[instructions[0]], - &Instruction::Truncate { value: v0, bit_size: 32, max_bit_size: 254 } - ); - assert_eq!( - &main.dfg[instructions[1]], - &Instruction::Cast(ValueId::test_new(5), Type::unsigned(32)) - ); + assert_eq!(instruction, &Instruction::Cast(v0, Type::unsigned(32))); } } From c88723f4f6a04f0485eff47133ec210bffa30201 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Fri, 5 Jan 2024 09:52:04 +0000 Subject: [PATCH 08/13] Update compiler/noirc_evaluator/src/ssa/ir/types.rs --- compiler/noirc_evaluator/src/ssa/ir/types.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler/noirc_evaluator/src/ssa/ir/types.rs b/compiler/noirc_evaluator/src/ssa/ir/types.rs index fa46ac40959..ae53c7705c2 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/types.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/types.rs @@ -19,6 +19,7 @@ pub enum NumericType { } impl NumericType { + /// Returns the bit size of the provided numeric type. pub(crate) fn bit_size(self: &NumericType) -> u32 { match self { NumericType::NativeField => FieldElement::max_num_bits(), From 555c508d324b1cde1d5345c93854e377a6916eb8 Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 5 Jan 2024 09:56:31 +0000 Subject: [PATCH 09/13] chore: add callstack to `Truncate` instruction from `from_field` call --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 2 +- compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 3 ++- compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index abddbfb74c7..931aee9d079 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -160,7 +160,7 @@ impl DataFlowGraph { call_stack: CallStack, ) -> InsertInstructionResult { use InsertInstructionResult::*; - match instruction.simplify(self, block, ctrl_typevars.clone()) { + match instruction.simplify(self, block, ctrl_typevars.clone(), &call_stack) { SimplifyResult::SimplifiedTo(simplification) => SimplifiedTo(simplification), SimplifyResult::SimplifiedToMultiple(simplification) => { SimplifiedToMultiple(simplification) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 6ab954f81b6..9691017f04b 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -405,6 +405,7 @@ impl Instruction { dfg: &mut DataFlowGraph, block: BasicBlockId, ctrl_typevars: Option>, + call_stack: &CallStack, ) -> SimplifyResult { use SimplifyResult::*; match self { @@ -548,7 +549,7 @@ impl Instruction { } } Instruction::Call { func, arguments } => { - simplify_call(*func, arguments, dfg, block, ctrl_typevars) + simplify_call(*func, arguments, dfg, block, ctrl_typevars, call_stack) } Instruction::EnableSideEffects { condition } => { if let Some(last) = dfg[block].instructions().last().copied() { diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 5b3e4a3bee5..edfc50a700f 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -1,7 +1,6 @@ use std::{collections::VecDeque, rc::Rc}; use acvm::{acir::BlackBoxFunc, BlackBoxResolutionError, FieldElement}; -use im::Vector; use iter_extended::vecmap; use num_bigint::BigUint; @@ -32,6 +31,7 @@ pub(super) fn simplify_call( dfg: &mut DataFlowGraph, block: BasicBlockId, ctrl_typevars: Option>, + call_stack: &CallStack, ) -> SimplifyResult { let intrinsic = match &dfg[func] { Value::Intrinsic(intrinsic) => *intrinsic, @@ -256,7 +256,7 @@ pub(super) fn simplify_call( truncate, block, Some(vec![incoming_type]), - Vector::default(), // needs a callstack + call_stack.clone(), ) .first(); From e0d43435c4fbdf429a26ed1af069ac2b61cae92d Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Fri, 5 Jan 2024 10:00:43 +0000 Subject: [PATCH 10/13] Update compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs --- compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 1b89647bad2..57c93c17fc4 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -323,7 +323,7 @@ mod test { // // fn main f0 { // b0(v0: u16): - // v1 = cast v1 as u32 + // v1 = cast v0 as u32 // } let ssa = ssa.fold_constants(); let main = ssa.main(); From 9a73673901d6b727b86c5c240090ef6765afd103 Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 5 Jan 2024 11:05:40 +0000 Subject: [PATCH 11/13] fix: ensure usage of safe casts in `context.rs` --- .../src/ssa/ssa_gen/context.rs | 32 +++++++++++-------- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 2 +- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 02c4f8e8413..2b0cf3354a7 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -275,6 +275,8 @@ impl<'a> FunctionContext<'a> { let bit_width = self.builder.numeric_constant(FieldElement::from(2_i128.pow(bit_size)), Type::field()); let sign_not = self.builder.insert_binary(one, BinaryOp::Sub, sign); + + // We use unsafe casts here, this is fine as we're casting to a `field` type. let as_field = self.builder.insert_cast(input, Type::field()); let sign_field = self.builder.insert_cast(sign, Type::field()); let positive_predicate = self.builder.insert_binary(sign_field, BinaryOp::Mul, as_field); @@ -310,12 +312,12 @@ impl<'a> FunctionContext<'a> { match operator { BinaryOpKind::Add | BinaryOpKind::Subtract => { // Result is computed modulo the bit size - let mut result = - self.builder.insert_truncate(result, bit_size, bit_size + 1); - result = self.builder.insert_cast(result, Type::unsigned(bit_size)); + let result = self.builder.insert_truncate(result, bit_size, bit_size + 1); + let result = + self.insert_safe_cast(result, Type::unsigned(bit_size), location); self.check_signed_overflow(result, lhs, rhs, operator, bit_size, location); - self.builder.insert_cast(result, result_type) + self.insert_safe_cast(result, result_type, location) } BinaryOpKind::Multiply => { // Result is computed modulo the bit size @@ -324,7 +326,7 @@ impl<'a> FunctionContext<'a> { result = self.builder.insert_truncate(result, bit_size, 2 * bit_size); self.check_signed_overflow(result, lhs, rhs, operator, bit_size, location); - self.builder.insert_cast(result, result_type) + self.insert_safe_cast(result, result_type, location) } BinaryOpKind::ShiftLeft | BinaryOpKind::ShiftRight => { self.check_shift_overflow(result, rhs, bit_size, location, true) @@ -375,8 +377,11 @@ impl<'a> FunctionContext<'a> { is_signed: bool, ) -> ValueId { let one = self.builder.numeric_constant(FieldElement::one(), Type::bool()); - let rhs = - if is_signed { self.builder.insert_cast(rhs, Type::unsigned(bit_size)) } else { rhs }; + let rhs = if is_signed { + self.insert_safe_cast(rhs, Type::unsigned(bit_size), location) + } else { + rhs + }; // Bit-shift with a negative number is an overflow if is_signed { // We compute the sign of rhs. @@ -432,8 +437,8 @@ impl<'a> FunctionContext<'a> { Type::unsigned(bit_size), ); // We compute the sign of the operands. The overflow checks for signed integers depends on these signs - let lhs_as_unsigned = self.builder.insert_cast(lhs, Type::unsigned(bit_size)); - let rhs_as_unsigned = self.builder.insert_cast(rhs, Type::unsigned(bit_size)); + let lhs_as_unsigned = self.insert_safe_cast(lhs, Type::unsigned(bit_size), location); + let rhs_as_unsigned = self.insert_safe_cast(rhs, Type::unsigned(bit_size), location); let lhs_sign = self.builder.insert_binary(lhs_as_unsigned, BinaryOp::Lt, half_width); let mut rhs_sign = self.builder.insert_binary(rhs_as_unsigned, BinaryOp::Lt, half_width); let message = if is_sub { @@ -470,12 +475,13 @@ impl<'a> FunctionContext<'a> { assert_message: Some(message.clone()), }; self.builder.set_location(location).insert_instruction(size_overflow, None); - let product = self.builder.insert_cast(product_field, Type::unsigned(bit_size)); + let product = + self.insert_safe_cast(product_field, Type::unsigned(bit_size), location); // Then we check the signed product fits in a signed integer of bit_size-bits let not_same = self.builder.insert_binary(one, BinaryOp::Sub, same_sign); let not_same_sign_field = - self.builder.insert_cast(not_same, Type::unsigned(bit_size)); + self.insert_safe_cast(not_same, Type::unsigned(bit_size), location); let positive_maximum_with_offset = self.builder.insert_binary(half_width, BinaryOp::Add, not_same_sign_field); let product_overflow_check = @@ -674,7 +680,7 @@ impl<'a> FunctionContext<'a> { value: ValueId, typ: Type, location: Location, - ) -> Result { + ) -> ValueId { self.builder.set_location(location); // To ensure that `value` is a valid `typ`, we insert an `Instruction::Truncate` instruction beforehand if @@ -687,7 +693,7 @@ impl<'a> FunctionContext<'a> { value }; - Ok(self.builder.insert_cast(truncated_value, typ).into()) + self.builder.insert_cast(truncated_value, typ) } /// Create a const offset of an address for an array load or store diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 2358ccdc776..0b8c3a37ef9 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -435,7 +435,7 @@ impl<'a> FunctionContext<'a> { let lhs = self.codegen_non_tuple_expression(&cast.lhs)?; let typ = Self::convert_non_tuple_type(&cast.r#type); - self.insert_safe_cast(lhs, typ, cast.location) + Ok(self.insert_safe_cast(lhs, typ, cast.location).into()) } /// Codegens a for loop, creating three new blocks in the process. From 474aeaa6f1e0d0eb3a573fe0cc3e65010b7f75ba Mon Sep 17 00:00:00 2001 From: jfecher Date: Mon, 8 Jan 2024 13:08:54 -0500 Subject: [PATCH 12/13] Update compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs --- compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 63f7a62ada8..b40093291cf 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -674,7 +674,7 @@ impl<'a> FunctionContext<'a> { /// Compared to `self.builder.insert_cast`, this version will automatically truncate `value` to be a valid `typ`. pub(super) fn insert_safe_cast( &mut self, - value: ValueId, + mut value: ValueId, typ: Type, location: Location, ) -> ValueId { From 7da299cb8d48f97f72ddd3e3369839c853223ef9 Mon Sep 17 00:00:00 2001 From: jfecher Date: Mon, 8 Jan 2024 13:09:00 -0500 Subject: [PATCH 13/13] Update compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs --- compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index b40093291cf..b34b667c31a 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -684,13 +684,11 @@ impl<'a> FunctionContext<'a> { // we're narrowing the type size. let incoming_type_size = self.builder.type_of_value(value).bit_size(); let target_type_size = typ.bit_size(); - let truncated_value = if target_type_size < incoming_type_size { - self.builder.insert_truncate(value, target_type_size, incoming_type_size) - } else { - value - }; + if target_type_size < incoming_type_size { + value = self.builder.insert_truncate(value, target_type_size, incoming_type_size); + } - self.builder.insert_cast(truncated_value, typ) + self.builder.insert_cast(value, typ) } /// Create a const offset of an address for an array load or store