From 6331a91dab2a3713b8b808fcaeb8b48361217b42 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 9 Jan 2025 15:54:44 -0300 Subject: [PATCH 01/24] Add some unchecked math operations --- compiler/noirc_evaluator/src/acir/mod.rs | 6 ++-- .../src/brillig/brillig_gen/brillig_block.rs | 6 ++-- .../noirc_evaluator/src/ssa/ir/instruction.rs | 10 ++++-- .../src/ssa/ir/instruction/binary.rs | 33 ++++++++++++------- 4 files changed, 35 insertions(+), 20 deletions(-) diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index e57f5d18830..fd78b605068 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -1949,9 +1949,9 @@ impl<'a> Context<'a> { let bit_count = binary_type.bit_size::(); let num_type = binary_type.to_numeric_type(); let result = match binary.operator { - BinaryOp::Add => self.acir_context.add_var(lhs, rhs), - BinaryOp::Sub => self.acir_context.sub_var(lhs, rhs), - BinaryOp::Mul => self.acir_context.mul_var(lhs, rhs), + BinaryOp::Add | BinaryOp::UncheckedAdd => self.acir_context.add_var(lhs, rhs), + BinaryOp::Sub | BinaryOp::UncheckedSub => self.acir_context.sub_var(lhs, rhs), + BinaryOp::Mul | BinaryOp::UncheckedMul => self.acir_context.mul_var(lhs, rhs), BinaryOp::Div => self.acir_context.div_var( lhs, rhs, diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 31c99bf433e..611dc2aede7 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -1318,9 +1318,9 @@ impl<'block> BrilligBlock<'block> { BrilligBinaryOp::Modulo } } - BinaryOp::Add => BrilligBinaryOp::Add, - BinaryOp::Sub => BrilligBinaryOp::Sub, - BinaryOp::Mul => BrilligBinaryOp::Mul, + BinaryOp::Add | BinaryOp::UncheckedAdd => BrilligBinaryOp::Add, + BinaryOp::Sub | BinaryOp::UncheckedSub => BrilligBinaryOp::Sub, + BinaryOp::Mul | BinaryOp::UncheckedMul => BrilligBinaryOp::Mul, BinaryOp::Eq => BrilligBinaryOp::Equals, BinaryOp::Lt => { if is_signed { diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 17cde96cddc..94061838cf2 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -415,7 +415,10 @@ impl Instruction { BinaryOp::Add | BinaryOp::Sub | BinaryOp::Mul | BinaryOp::Div | BinaryOp::Mod => { true } - BinaryOp::Eq + BinaryOp::UncheckedAdd + | BinaryOp::UncheckedSub + | BinaryOp::UncheckedMul + | BinaryOp::Eq | BinaryOp::Lt | BinaryOp::And | BinaryOp::Or @@ -575,7 +578,10 @@ impl Instruction { // for unsigned types (here we assume the type of binary.lhs is the same) dfg.type_of_value(binary.rhs).is_unsigned() } - BinaryOp::Eq + BinaryOp::UncheckedAdd + | BinaryOp::UncheckedSub + | BinaryOp::UncheckedMul + | BinaryOp::Eq | BinaryOp::Lt | BinaryOp::And | BinaryOp::Or diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs index 28e58e2cbb1..93d76bd395c 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs @@ -16,10 +16,16 @@ use super::{ pub(crate) enum BinaryOp { /// Addition of lhs + rhs. Add, + /// Addition of lhs + rhs without an overflow check. + UncheckedAdd, /// Subtraction of lhs - rhs. Sub, + /// Subtraction of lhs - rhs without an overflow check. + UncheckedSub, /// Multiplication of lhs * rhs. Mul, + /// Multiplication of lhs * rhs without an overflow check. + UncheckedMul, /// Division of lhs / rhs. Div, /// Modulus of lhs % rhs. @@ -49,8 +55,11 @@ impl std::fmt::Display for BinaryOp { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { BinaryOp::Add => write!(f, "add"), + BinaryOp::UncheckedAdd => write!(f, "add_unchecked"), BinaryOp::Sub => write!(f, "sub"), + BinaryOp::UncheckedSub => write!(f, "sub_unchecked"), BinaryOp::Mul => write!(f, "mul"), + BinaryOp::UncheckedMul => write!(f, "mul_unchecked"), BinaryOp::Div => write!(f, "div"), BinaryOp::Eq => write!(f, "eq"), BinaryOp::Mod => write!(f, "mod"), @@ -107,7 +116,7 @@ impl Binary { let rhs_is_one = rhs.map_or(false, |rhs| rhs.is_one()); match self.operator { - BinaryOp::Add => { + BinaryOp::Add | BinaryOp::UncheckedAdd => { if lhs_is_zero { return SimplifyResult::SimplifiedTo(self.rhs); } @@ -115,12 +124,12 @@ impl Binary { return SimplifyResult::SimplifiedTo(self.lhs); } } - BinaryOp::Sub => { + BinaryOp::Sub | BinaryOp::UncheckedSub => { if rhs_is_zero { return SimplifyResult::SimplifiedTo(self.lhs); } } - BinaryOp::Mul => { + BinaryOp::Mul | BinaryOp::UncheckedMul => { if lhs_is_one { return SimplifyResult::SimplifiedTo(self.rhs); } @@ -470,9 +479,9 @@ fn truncate(int: u128, bit_size: u32) -> u128 { impl BinaryOp { fn get_field_function(self) -> Option FieldElement> { match self { - BinaryOp::Add => Some(std::ops::Add::add), - BinaryOp::Sub => Some(std::ops::Sub::sub), - BinaryOp::Mul => Some(std::ops::Mul::mul), + BinaryOp::Add | BinaryOp::UncheckedAdd => Some(std::ops::Add::add), + BinaryOp::Sub | BinaryOp::UncheckedSub => Some(std::ops::Sub::sub), + BinaryOp::Mul | BinaryOp::UncheckedMul => Some(std::ops::Mul::mul), BinaryOp::Div => Some(std::ops::Div::div), BinaryOp::Eq => Some(|x, y| (x == y).into()), BinaryOp::Lt => Some(|x, y| (x < y).into()), @@ -488,9 +497,9 @@ impl BinaryOp { fn get_u128_function(self) -> fn(u128, u128) -> Option { match self { - BinaryOp::Add => u128::checked_add, - BinaryOp::Sub => u128::checked_sub, - BinaryOp::Mul => u128::checked_mul, + BinaryOp::Add | BinaryOp::UncheckedAdd => u128::checked_add, + BinaryOp::Sub | BinaryOp::UncheckedSub => u128::checked_sub, + BinaryOp::Mul | BinaryOp::UncheckedMul => u128::checked_mul, BinaryOp::Div => u128::checked_div, BinaryOp::Mod => u128::checked_rem, BinaryOp::And => |x, y| Some(x & y), @@ -505,9 +514,9 @@ impl BinaryOp { fn get_i128_function(self) -> fn(i128, i128) -> Option { match self { - BinaryOp::Add => i128::checked_add, - BinaryOp::Sub => i128::checked_sub, - BinaryOp::Mul => i128::checked_mul, + BinaryOp::Add | BinaryOp::UncheckedAdd => i128::checked_add, + BinaryOp::Sub | BinaryOp::UncheckedSub => i128::checked_sub, + BinaryOp::Mul | BinaryOp::UncheckedMul => i128::checked_mul, BinaryOp::Div => i128::checked_div, BinaryOp::Mod => i128::checked_rem, BinaryOp::And => |x, y| Some(x & y), From 3bb465a587f385ae6f86bcbeeda7690bed9d7eb8 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 9 Jan 2025 15:55:56 -0300 Subject: [PATCH 02/24] Use unchecked add in `make_offset` --- 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 e89d1d2a0c3..9c3f3814b02 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -610,7 +610,7 @@ impl<'a> FunctionContext<'a> { if offset != 0 { let typ = self.builder.type_of_value(address).unwrap_numeric(); let offset = self.builder.numeric_constant(offset, typ); - address = self.builder.insert_binary(address, BinaryOp::Add, offset); + address = self.builder.insert_binary(address, BinaryOp::UncheckedAdd, offset); } address } From 41ee955cd365d47071783114219aba63c2c83f11 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 9 Jan 2025 15:59:38 -0300 Subject: [PATCH 03/24] Use correct names when printing --- compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs index 93d76bd395c..a9b7c28549c 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs @@ -55,11 +55,11 @@ impl std::fmt::Display for BinaryOp { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { BinaryOp::Add => write!(f, "add"), - BinaryOp::UncheckedAdd => write!(f, "add_unchecked"), + BinaryOp::UncheckedAdd => write!(f, "unchecked_add"), BinaryOp::Sub => write!(f, "sub"), - BinaryOp::UncheckedSub => write!(f, "sub_unchecked"), + BinaryOp::UncheckedSub => write!(f, "unchecked_sub"), BinaryOp::Mul => write!(f, "mul"), - BinaryOp::UncheckedMul => write!(f, "mul_unchecked"), + BinaryOp::UncheckedMul => write!(f, "unchecked_mul"), BinaryOp::Div => write!(f, "div"), BinaryOp::Eq => write!(f, "eq"), BinaryOp::Mod => write!(f, "mod"), From 63af35134361be437be03e52a649340470f5bee7 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 9 Jan 2025 16:02:06 -0300 Subject: [PATCH 04/24] Parse unchecked ops --- compiler/noirc_evaluator/src/ssa/parser/mod.rs | 3 +++ .../noirc_evaluator/src/ssa/parser/tests.rs | 18 +++++++++++++++++- .../noirc_evaluator/src/ssa/parser/token.rs | 9 +++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/parser/mod.rs b/compiler/noirc_evaluator/src/ssa/parser/mod.rs index 96aef1f3c32..6fc208ce41c 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/mod.rs @@ -298,6 +298,9 @@ impl<'a> Parser<'a> { Token::Keyword(Keyword::Xor) => BinaryOp::Xor, Token::Keyword(Keyword::Shl) => BinaryOp::Shl, Token::Keyword(Keyword::Shr) => BinaryOp::Shr, + Token::Keyword(Keyword::UncheckedAdd) => BinaryOp::UncheckedAdd, + Token::Keyword(Keyword::UncheckedSub) => BinaryOp::UncheckedSub, + Token::Keyword(Keyword::UncheckedMul) => BinaryOp::UncheckedMul, _ => return Ok(None), }; diff --git a/compiler/noirc_evaluator/src/ssa/parser/tests.rs b/compiler/noirc_evaluator/src/ssa/parser/tests.rs index b5aac13cfd8..ee96d5d82c8 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/tests.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/tests.rs @@ -326,7 +326,23 @@ fn test_array_get_set_bug() { #[test] fn test_binary() { - for op in ["add", "sub", "mul", "div", "eq", "mod", "lt", "and", "or", "xor", "shl", "shr"] { + for op in [ + "add", + "sub", + "mul", + "div", + "eq", + "mod", + "lt", + "and", + "or", + "xor", + "shl", + "shr", + "unchecked_add", + "unchecked_sub", + "unchecked_mul", + ] { let src = format!( " acir(inline) fn main f0 {{ diff --git a/compiler/noirc_evaluator/src/ssa/parser/token.rs b/compiler/noirc_evaluator/src/ssa/parser/token.rs index 83a2a1d1ed2..eb09209466d 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/token.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/token.rs @@ -160,6 +160,9 @@ pub(crate) enum Keyword { Then, To, Truncate, + UncheckedAdd, + UncheckedSub, + UncheckedMul, Value, Xor, } @@ -217,6 +220,9 @@ impl Keyword { "then" => Keyword::Then, "to" => Keyword::To, "truncate" => Keyword::Truncate, + "unchecked_add" => Keyword::UncheckedAdd, + "unchecked_sub" => Keyword::UncheckedSub, + "unchecked_mul" => Keyword::UncheckedMul, "value" => Keyword::Value, "xor" => Keyword::Xor, _ => return None, @@ -278,6 +284,9 @@ impl Display for Keyword { Keyword::Then => write!(f, "then"), Keyword::To => write!(f, "to"), Keyword::Truncate => write!(f, "truncate"), + Keyword::UncheckedAdd => write!(f, "unchecked_add"), + Keyword::UncheckedSub => write!(f, "unchecked_sub"), + Keyword::UncheckedMul => write!(f, "unchecked_mul"), Keyword::Value => write!(f, "value"), Keyword::Xor => write!(f, "xor"), } From a3087673e2ab26198544f48f015f1c186c016c10 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 9 Jan 2025 16:10:06 -0300 Subject: [PATCH 05/24] Induction variable now uses unchecked add --- compiler/noirc_evaluator/src/ssa/opt/unrolling.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index ab4256197b9..1caa5d14ce2 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -619,10 +619,17 @@ impl Loop { let header = &function.dfg[self.header]; let induction_var = header.parameters()[0]; - back.instructions().iter().filter(|instruction| { - let instruction = &function.dfg[**instruction]; - matches!(instruction, Instruction::Binary(Binary { lhs, operator: BinaryOp::Add, rhs: _ }) if *lhs == induction_var) - }).count() + back.instructions() + .iter() + .filter(|instruction| { + let instruction = &function.dfg[**instruction]; + matches!(instruction, + Instruction::Binary(Binary { lhs, operator: BinaryOp::Add, rhs: _ }) | + Instruction::Binary(Binary { lhs, operator: BinaryOp::UncheckedAdd, rhs: _ }) + if *lhs == induction_var + ) + }) + .count() } /// Decide if this loop is small enough that it can be inlined in a way that the number From 199a980bf967d225f11ea956cb66bc2ff59369b1 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 9 Jan 2025 16:15:00 -0300 Subject: [PATCH 06/24] A few more cases where we need to check for unchecked ops --- compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs | 5 ++++- .../src/ssa/opt/remove_enable_side_effects.rs | 9 ++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index c1f2a6b88e5..09083369d4c 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -233,7 +233,10 @@ impl<'f> LoopInvariantContext<'f> { } } Instruction::Binary(binary) => { - if !matches!(binary.operator, BinaryOp::Add | BinaryOp::Mul) { + if !matches!( + binary.operator, + BinaryOp::Add | BinaryOp::UncheckedAdd | BinaryOp::Mul | BinaryOp::UncheckedMul + ) { return false; } diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs index a22232ba49a..efe30748d49 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs @@ -126,9 +126,12 @@ impl Context { use Instruction::*; match instruction { Binary(binary) => match binary.operator { - BinaryOp::Add | BinaryOp::Sub | BinaryOp::Mul => { - dfg.type_of_value(binary.lhs).is_unsigned() - } + BinaryOp::Add + | BinaryOp::UncheckedAdd + | BinaryOp::Sub + | BinaryOp::UncheckedSub + | BinaryOp::Mul + | BinaryOp::UncheckedMul => dfg.type_of_value(binary.lhs).is_unsigned(), BinaryOp::Div | BinaryOp::Mod => { if let Some(rhs) = dfg.get_numeric_constant(binary.rhs) { rhs == FieldElement::zero() From f279e3dc045ce0d5ff592b6e6fc82d6b962fec08 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 9 Jan 2025 16:27:57 -0300 Subject: [PATCH 07/24] A few more --- compiler/noirc_evaluator/src/acir/mod.rs | 1 + compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs | 4 ++-- compiler/noirc_evaluator/src/ssa/ir/instruction/constrain.rs | 1 + 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index fd78b605068..55c4c3864c8 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -2071,6 +2071,7 @@ impl<'a> Context<'a> { if matches!( &dfg[*instruction], Instruction::Binary(Binary { operator: BinaryOp::Sub, .. }) + | Instruction::Binary(Binary { operator: BinaryOp::UncheckedSub, .. }) ) { // Subtractions must first have the integer modulus added before truncation can be // applied. This is done in order to prevent underflow. diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs index a9b7c28549c..39a177dd0ba 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs @@ -150,7 +150,7 @@ impl Binary { if let Instruction::Binary(Binary { lhs, rhs, operator }) = dfg[*instruction] { - if operator == BinaryOp::Mul + if (operator == BinaryOp::Mul || operator == BinaryOp::UncheckedMul) && (dfg.resolve(self.lhs) == dfg.resolve(lhs) || dfg.resolve(self.lhs) == dfg.resolve(rhs)) { @@ -165,7 +165,7 @@ impl Binary { if let Instruction::Binary(Binary { lhs, rhs, operator }) = dfg[*instruction] { - if operator == BinaryOp::Mul + if (operator == BinaryOp::Mul || operator == BinaryOp::UncheckedMul) && (dfg.resolve(self.rhs) == dfg.resolve(lhs) || dfg.resolve(self.rhs) == dfg.resolve(rhs)) { diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/constrain.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/constrain.rs index 5ae6a642a57..66d5ab74695 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/constrain.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/constrain.rs @@ -45,6 +45,7 @@ pub(super) fn decompose_constrain( } Instruction::Binary(Binary { lhs, rhs, operator: BinaryOp::Mul }) + | Instruction::Binary(Binary { lhs, rhs, operator: BinaryOp::UncheckedMul }) if constant.is_one() && dfg.type_of_value(lhs) == Type::bool() => { // Replace an equality assertion on a boolean multiplication From a2beb7854de280ca8e77e3b72660f1682040f247 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 9 Jan 2025 17:15:12 -0300 Subject: [PATCH 08/24] Use a boolean instead of another variant --- compiler/noirc_evaluator/src/acir/mod.rs | 15 +++-- .../src/brillig/brillig_gen/brillig_block.rs | 6 +- .../brillig/brillig_gen/variable_liveness.rs | 12 ++-- .../noirc_evaluator/src/ssa/ir/instruction.rs | 26 ++++---- .../src/ssa/ir/instruction/binary.rs | 62 ++++++++--------- .../src/ssa/ir/instruction/call.rs | 61 +++++++++++++---- .../src/ssa/ir/instruction/constrain.rs | 3 +- compiler/noirc_evaluator/src/ssa/opt/die.rs | 6 +- .../src/ssa/opt/flatten_cfg.rs | 39 +++++++++-- .../src/ssa/opt/flatten_cfg/value_merger.rs | 11 +++- .../noirc_evaluator/src/ssa/opt/inlining.rs | 7 +- .../src/ssa/opt/loop_invariant.rs | 5 +- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- .../src/ssa/opt/remove_bit_shifts.rs | 43 +++++++++--- .../src/ssa/opt/remove_enable_side_effects.rs | 22 +++---- .../noirc_evaluator/src/ssa/opt/unrolling.rs | 3 +- .../noirc_evaluator/src/ssa/parser/mod.rs | 12 ++-- .../src/ssa/ssa_gen/context.rs | 66 ++++++++++++++----- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 15 ++++- .../src/ssa/ssa_gen/program.rs | 4 +- 20 files changed, 276 insertions(+), 144 deletions(-) diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index 55c4c3864c8..fcfb04eb5cc 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -1949,9 +1949,9 @@ impl<'a> Context<'a> { let bit_count = binary_type.bit_size::(); let num_type = binary_type.to_numeric_type(); let result = match binary.operator { - BinaryOp::Add | BinaryOp::UncheckedAdd => self.acir_context.add_var(lhs, rhs), - BinaryOp::Sub | BinaryOp::UncheckedSub => self.acir_context.sub_var(lhs, rhs), - BinaryOp::Mul | BinaryOp::UncheckedMul => self.acir_context.mul_var(lhs, rhs), + BinaryOp::Add { .. } => self.acir_context.add_var(lhs, rhs), + BinaryOp::Sub { .. } => self.acir_context.sub_var(lhs, rhs), + BinaryOp::Mul { .. } => self.acir_context.mul_var(lhs, rhs), BinaryOp::Div => self.acir_context.div_var( lhs, rhs, @@ -2070,8 +2070,7 @@ impl<'a> Context<'a> { Value::Instruction { instruction, .. } => { if matches!( &dfg[*instruction], - Instruction::Binary(Binary { operator: BinaryOp::Sub, .. }) - | Instruction::Binary(Binary { operator: BinaryOp::UncheckedSub, .. }) + Instruction::Binary(Binary { operator: BinaryOp::Sub { .. }, .. }) ) { // Subtractions must first have the integer modulus added before truncation can be // applied. This is done in order to prevent underflow. @@ -3160,7 +3159,11 @@ mod test { let func_with_nested_call_v1 = builder.add_parameter(Type::field()); let two = builder.field_constant(2u128); - let v0_plus_two = builder.insert_binary(func_with_nested_call_v0, BinaryOp::Add, two); + let v0_plus_two = builder.insert_binary( + func_with_nested_call_v0, + BinaryOp::Add { unchecked: false }, + two, + ); let foo_id = Id::test_new(2); let foo_call = builder.import_function(foo_id); diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 611dc2aede7..c986aefb349 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -1318,9 +1318,9 @@ impl<'block> BrilligBlock<'block> { BrilligBinaryOp::Modulo } } - BinaryOp::Add | BinaryOp::UncheckedAdd => BrilligBinaryOp::Add, - BinaryOp::Sub | BinaryOp::UncheckedSub => BrilligBinaryOp::Sub, - BinaryOp::Mul | BinaryOp::UncheckedMul => BrilligBinaryOp::Mul, + BinaryOp::Add { .. } => BrilligBinaryOp::Add, + BinaryOp::Sub { .. } => BrilligBinaryOp::Sub, + BinaryOp::Mul { .. } => BrilligBinaryOp::Mul, BinaryOp::Eq => BrilligBinaryOp::Equals, BinaryOp::Lt => { if is_signed { diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs index d6851a9ecf9..1c908f70145 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs @@ -382,14 +382,14 @@ mod test { builder.switch_to_block(b2); let twenty_seven = builder.field_constant(27u128); - let v7 = builder.insert_binary(v0, BinaryOp::Add, twenty_seven); + let v7 = builder.insert_binary(v0, BinaryOp::Add { unchecked: false }, twenty_seven); builder.insert_store(v3, v7); builder.terminate_with_jmp(b3, vec![]); builder.switch_to_block(b1); - let v6 = builder.insert_binary(v1, BinaryOp::Add, twenty_seven); + let v6 = builder.insert_binary(v1, BinaryOp::Add { unchecked: false }, twenty_seven); builder.insert_store(v3, v6); builder.terminate_with_jmp(b3, vec![]); @@ -501,7 +501,7 @@ mod test { builder.switch_to_block(b2); - let v6 = builder.insert_binary(v4, BinaryOp::Mul, v4); + let v6 = builder.insert_binary(v4, BinaryOp::Mul { unchecked: false }, v4); builder.terminate_with_jmp(b4, vec![v0]); @@ -526,7 +526,7 @@ mod test { let v12 = builder.insert_load(v3, Type::field()); - let v13 = builder.insert_binary(v12, BinaryOp::Add, v6); + let v13 = builder.insert_binary(v12, BinaryOp::Add { unchecked: false }, v6); builder.insert_store(v3, v13); @@ -535,13 +535,13 @@ mod test { builder.switch_to_block(b8); let one = builder.field_constant(1u128); - let v15 = builder.insert_binary(v7, BinaryOp::Add, one); + let v15 = builder.insert_binary(v7, BinaryOp::Add { unchecked: false }, one); builder.terminate_with_jmp(b4, vec![v15]); builder.switch_to_block(b6); - let v16 = builder.insert_binary(v4, BinaryOp::Add, one); + let v16 = builder.insert_binary(v4, BinaryOp::Add { unchecked: false }, one); builder.terminate_with_jmp(b1, vec![v16]); diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 94061838cf2..67210d59aa8 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -412,12 +412,14 @@ impl Instruction { // Some binary math can overflow or underflow Binary(binary) => match binary.operator { - BinaryOp::Add | BinaryOp::Sub | BinaryOp::Mul | BinaryOp::Div | BinaryOp::Mod => { - true - } - BinaryOp::UncheckedAdd - | BinaryOp::UncheckedSub - | BinaryOp::UncheckedMul + BinaryOp::Add { unchecked: false } + | BinaryOp::Sub { unchecked: false } + | BinaryOp::Mul { unchecked: false } + | BinaryOp::Div + | BinaryOp::Mod => true, + BinaryOp::Add { unchecked: true } + | BinaryOp::Sub { unchecked: true } + | BinaryOp::Mul { unchecked: true } | BinaryOp::Eq | BinaryOp::Lt | BinaryOp::And @@ -569,18 +571,18 @@ impl Instruction { match self { Instruction::Binary(binary) => { match binary.operator { - BinaryOp::Add - | BinaryOp::Sub - | BinaryOp::Mul + BinaryOp::Add { unchecked: false } + | BinaryOp::Sub { unchecked: false } + | BinaryOp::Mul { unchecked: false } | BinaryOp::Div | BinaryOp::Mod => { // Some binary math can overflow or underflow, but this is only the case // for unsigned types (here we assume the type of binary.lhs is the same) dfg.type_of_value(binary.rhs).is_unsigned() } - BinaryOp::UncheckedAdd - | BinaryOp::UncheckedSub - | BinaryOp::UncheckedMul + BinaryOp::Add { unchecked: true } + | BinaryOp::Sub { unchecked: true } + | BinaryOp::Mul { unchecked: true } | BinaryOp::Eq | BinaryOp::Lt | BinaryOp::And diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs index 39a177dd0ba..7cd3a92966d 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs @@ -15,17 +15,11 @@ use super::{ #[derive(Debug, PartialEq, Eq, Hash, Copy, Clone, Serialize, Deserialize)] pub(crate) enum BinaryOp { /// Addition of lhs + rhs. - Add, - /// Addition of lhs + rhs without an overflow check. - UncheckedAdd, + Add { unchecked: bool }, /// Subtraction of lhs - rhs. - Sub, - /// Subtraction of lhs - rhs without an overflow check. - UncheckedSub, + Sub { unchecked: bool }, /// Multiplication of lhs * rhs. - Mul, - /// Multiplication of lhs * rhs without an overflow check. - UncheckedMul, + Mul { unchecked: bool }, /// Division of lhs / rhs. Div, /// Modulus of lhs % rhs. @@ -54,12 +48,12 @@ pub(crate) enum BinaryOp { impl std::fmt::Display for BinaryOp { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - BinaryOp::Add => write!(f, "add"), - BinaryOp::UncheckedAdd => write!(f, "unchecked_add"), - BinaryOp::Sub => write!(f, "sub"), - BinaryOp::UncheckedSub => write!(f, "unchecked_sub"), - BinaryOp::Mul => write!(f, "mul"), - BinaryOp::UncheckedMul => write!(f, "unchecked_mul"), + BinaryOp::Add { unchecked: false } => write!(f, "add"), + BinaryOp::Add { unchecked: true } => write!(f, "unchecked_add"), + BinaryOp::Sub { unchecked: false } => write!(f, "sub"), + BinaryOp::Sub { unchecked: true } => write!(f, "unchecked_sub"), + BinaryOp::Mul { unchecked: false } => write!(f, "mul"), + BinaryOp::Mul { unchecked: true } => write!(f, "unchecked_mul"), BinaryOp::Div => write!(f, "div"), BinaryOp::Eq => write!(f, "eq"), BinaryOp::Mod => write!(f, "mod"), @@ -116,7 +110,7 @@ impl Binary { let rhs_is_one = rhs.map_or(false, |rhs| rhs.is_one()); match self.operator { - BinaryOp::Add | BinaryOp::UncheckedAdd => { + BinaryOp::Add { .. } => { if lhs_is_zero { return SimplifyResult::SimplifiedTo(self.rhs); } @@ -124,12 +118,12 @@ impl Binary { return SimplifyResult::SimplifiedTo(self.lhs); } } - BinaryOp::Sub | BinaryOp::UncheckedSub => { + BinaryOp::Sub { .. } => { if rhs_is_zero { return SimplifyResult::SimplifiedTo(self.lhs); } } - BinaryOp::Mul | BinaryOp::UncheckedMul => { + BinaryOp::Mul { .. } => { if lhs_is_one { return SimplifyResult::SimplifiedTo(self.rhs); } @@ -150,7 +144,7 @@ impl Binary { if let Instruction::Binary(Binary { lhs, rhs, operator }) = dfg[*instruction] { - if (operator == BinaryOp::Mul || operator == BinaryOp::UncheckedMul) + if matches!(operator, BinaryOp::Mul { .. }) && (dfg.resolve(self.lhs) == dfg.resolve(lhs) || dfg.resolve(self.lhs) == dfg.resolve(rhs)) { @@ -165,7 +159,7 @@ impl Binary { if let Instruction::Binary(Binary { lhs, rhs, operator }) = dfg[*instruction] { - if (operator == BinaryOp::Mul || operator == BinaryOp::UncheckedMul) + if matches!(operator, BinaryOp::Mul { .. }) && (dfg.resolve(self.rhs) == dfg.resolve(lhs) || dfg.resolve(self.rhs) == dfg.resolve(rhs)) { @@ -256,7 +250,9 @@ impl Binary { } if operand_type == NumericType::bool() { // Boolean AND is equivalent to multiplication, which is a cheaper operation. - let instruction = Instruction::binary(BinaryOp::Mul, self.lhs, self.rhs); + // TODO: should this be unchecked? + let instruction = + Instruction::binary(BinaryOp::Mul { unchecked: false }, self.lhs, self.rhs); return SimplifyResult::SimplifiedToInstruction(instruction); } if operand_type.is_unsigned() { @@ -340,14 +336,14 @@ impl Binary { let max_rhs_bits = dfg.get_value_max_num_bits(self.rhs); let msg = match self.operator { - BinaryOp::Add => { + BinaryOp::Add { unchecked: false } => { if std::cmp::max(max_lhs_bits, max_rhs_bits) < bit_size { // `lhs` and `rhs` have both been casted up from smaller types and so cannot overflow. return None; } "attempt to add with overflow" } - BinaryOp::Sub => { + BinaryOp::Sub { unchecked: false } => { if dfg.is_constant(self.lhs) && max_lhs_bits > max_rhs_bits { // `lhs` is a fixed constant and `rhs` is restricted such that `lhs - rhs > 0` // Note strict inequality as `rhs > lhs` while `max_lhs_bits == max_rhs_bits` is possible. @@ -355,7 +351,7 @@ impl Binary { } "attempt to subtract with overflow" } - BinaryOp::Mul => { + BinaryOp::Mul { unchecked: false } => { if bit_size == 1 || max_lhs_bits + max_rhs_bits <= bit_size || max_lhs_bits == 1 @@ -479,9 +475,9 @@ fn truncate(int: u128, bit_size: u32) -> u128 { impl BinaryOp { fn get_field_function(self) -> Option FieldElement> { match self { - BinaryOp::Add | BinaryOp::UncheckedAdd => Some(std::ops::Add::add), - BinaryOp::Sub | BinaryOp::UncheckedSub => Some(std::ops::Sub::sub), - BinaryOp::Mul | BinaryOp::UncheckedMul => Some(std::ops::Mul::mul), + BinaryOp::Add { .. } => Some(std::ops::Add::add), + BinaryOp::Sub { .. } => Some(std::ops::Sub::sub), + BinaryOp::Mul { .. } => Some(std::ops::Mul::mul), BinaryOp::Div => Some(std::ops::Div::div), BinaryOp::Eq => Some(|x, y| (x == y).into()), BinaryOp::Lt => Some(|x, y| (x < y).into()), @@ -497,9 +493,9 @@ impl BinaryOp { fn get_u128_function(self) -> fn(u128, u128) -> Option { match self { - BinaryOp::Add | BinaryOp::UncheckedAdd => u128::checked_add, - BinaryOp::Sub | BinaryOp::UncheckedSub => u128::checked_sub, - BinaryOp::Mul | BinaryOp::UncheckedMul => u128::checked_mul, + BinaryOp::Add { .. } => u128::checked_add, + BinaryOp::Sub { .. } => u128::checked_sub, + BinaryOp::Mul { .. } => u128::checked_mul, BinaryOp::Div => u128::checked_div, BinaryOp::Mod => u128::checked_rem, BinaryOp::And => |x, y| Some(x & y), @@ -514,9 +510,9 @@ impl BinaryOp { fn get_i128_function(self) -> fn(i128, i128) -> Option { match self { - BinaryOp::Add | BinaryOp::UncheckedAdd => i128::checked_add, - BinaryOp::Sub | BinaryOp::UncheckedSub => i128::checked_sub, - BinaryOp::Mul | BinaryOp::UncheckedMul => i128::checked_mul, + BinaryOp::Add { .. } => i128::checked_add, + BinaryOp::Sub { .. } => i128::checked_sub, + BinaryOp::Mul { .. } => i128::checked_mul, BinaryOp::Div => i128::checked_div, BinaryOp::Mod => i128::checked_rem, BinaryOp::And => |x, y| Some(x & y), diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index f1ba21f1c1d..63c2434c8aa 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -142,8 +142,13 @@ pub(super) fn simplify_call( slice.push_back(*elem); } - let new_slice_length = - update_slice_length(arguments[0], dfg, BinaryOp::Add, block); + // TODO: should this use an unchecked add? + let new_slice_length = update_slice_length( + arguments[0], + dfg, + BinaryOp::Add { unchecked: false }, + block, + ); let new_slice = make_array(dfg, slice, element_type, block, call_stack); return SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]); @@ -161,7 +166,13 @@ pub(super) fn simplify_call( slice.push_front(*elem); } - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add, block); + // TODO: should this be unchecked? + let new_slice_length = update_slice_length( + arguments[0], + dfg, + BinaryOp::Add { unchecked: false }, + block, + ); let new_slice = make_array(dfg, slice, element_type, block, call_stack); SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) @@ -201,7 +212,13 @@ pub(super) fn simplify_call( slice.pop_front().expect("There are no elements in this slice to be removed") }); - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block); + // TODO: should this be unchecked? + let new_slice_length = update_slice_length( + arguments[0], + dfg, + BinaryOp::Sub { unchecked: false }, + block, + ); results.push(new_slice_length); @@ -234,7 +251,13 @@ pub(super) fn simplify_call( index += 1; } - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add, block); + // TODO: should this be unchecked? + let new_slice_length = update_slice_length( + arguments[0], + dfg, + BinaryOp::Add { unchecked: false }, + block, + ); let new_slice = make_array(dfg, slice, typ, block, call_stack); SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) @@ -272,7 +295,13 @@ pub(super) fn simplify_call( let new_slice = make_array(dfg, slice, typ, block, call_stack); results.insert(0, new_slice); - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block); + // TODO: should this be unchecked? + let new_slice_length = update_slice_length( + arguments[0], + dfg, + BinaryOp::Sub { unchecked: false }, + block, + ); results.insert(0, new_slice_length); @@ -408,7 +437,9 @@ fn simplify_slice_push_back( .insert_instruction_and_results(len_not_equals_capacity_instr, block, None, call_stack) .first(); - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add, block); + // TODO: should this be unchecked? + let new_slice_length = + update_slice_length(arguments[0], dfg, BinaryOp::Add { unchecked: false }, block); for elem in &arguments[2..] { slice.push_back(*elem); @@ -457,14 +488,20 @@ fn simplify_slice_pop_back( let element_count = element_types.len(); let mut results = VecDeque::with_capacity(element_count + 1); - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block); + // TODO: should this be unchecked? + let new_slice_length = + update_slice_length(arguments[0], dfg, BinaryOp::Sub { unchecked: false }, block); let element_size = dfg.make_constant((element_count as u128).into(), NumericType::length_type()); - let flattened_len_instr = Instruction::binary(BinaryOp::Mul, arguments[0], element_size); + // TODO: should this be unchecked? + let flattened_len_instr = + Instruction::binary(BinaryOp::Mul { unchecked: false }, arguments[0], element_size); let mut flattened_len = dfg.insert_instruction_and_results(flattened_len_instr, block, None, call_stack).first(); - flattened_len = update_slice_length(flattened_len, dfg, BinaryOp::Sub, block); + // TODO: should this be unchecked? + flattened_len = + update_slice_length(flattened_len, dfg, BinaryOp::Sub { unchecked: false }, block); // We must pop multiple elements in the case of a slice of tuples // Iterating through element types in reverse here since we're popping from the end @@ -478,7 +515,9 @@ fn simplify_slice_pop_back( .first(); results.push_front(get_last_elem); - flattened_len = update_slice_length(flattened_len, dfg, BinaryOp::Sub, block); + // TODO: should this be unchecked? + flattened_len = + update_slice_length(flattened_len, dfg, BinaryOp::Sub { unchecked: false }, block); } results.push_front(arguments[1]); diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/constrain.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/constrain.rs index 66d5ab74695..a3881419a83 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/constrain.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/constrain.rs @@ -44,8 +44,7 @@ pub(super) fn decompose_constrain( vec![Instruction::Constrain(lhs, rhs, msg.clone())] } - Instruction::Binary(Binary { lhs, rhs, operator: BinaryOp::Mul }) - | Instruction::Binary(Binary { lhs, rhs, operator: BinaryOp::UncheckedMul }) + Instruction::Binary(Binary { lhs, rhs, operator: BinaryOp::Mul { .. } }) if constant.is_one() && dfg.type_of_value(lhs) == Type::bool() => { // Replace an equality assertion on a boolean multiplication diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 7fdcb4c26c2..cda993deb37 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -507,16 +507,18 @@ fn apply_side_effects( let casted_condition = dfg.insert_instruction_and_results(cast, block_id, None, call_stack); let casted_condition = casted_condition.first(); + // TODO: should this be unchecked? let lhs = dfg.insert_instruction_and_results( - Instruction::binary(BinaryOp::Mul, lhs, casted_condition), + Instruction::binary(BinaryOp::Mul { unchecked: false }, lhs, casted_condition), block_id, None, call_stack, ); let lhs = lhs.first(); + // TODO: should this be unchecked? let rhs = dfg.insert_instruction_and_results( - Instruction::binary(BinaryOp::Mul, rhs, casted_condition), + Instruction::binary(BinaryOp::Mul { unchecked: false }, rhs, casted_condition), block_id, None, call_stack, diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 748867c7409..b36844386f2 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -659,12 +659,22 @@ impl<'f> Context<'f> { let cast = Instruction::Cast(condition, argument_type.unwrap_numeric()); let casted_condition = self.insert_instruction(cast, call_stack); + // TODO: should this be unchecked? let lhs = self.insert_instruction( - Instruction::binary(BinaryOp::Mul, lhs, casted_condition), + Instruction::binary( + BinaryOp::Mul { unchecked: false }, + lhs, + casted_condition, + ), call_stack, ); + // TODO: should this be unchecked? let rhs = self.insert_instruction( - Instruction::binary(BinaryOp::Mul, rhs, casted_condition), + Instruction::binary( + BinaryOp::Mul { unchecked: false }, + rhs, + casted_condition, + ), call_stack, ); @@ -704,8 +714,13 @@ impl<'f> Context<'f> { let cast = Instruction::Cast(condition, argument_type.unwrap_numeric()); let casted_condition = self.insert_instruction(cast, call_stack); + // TODO: should this be unchecked? let value = self.insert_instruction( - Instruction::binary(BinaryOp::Mul, value, casted_condition), + Instruction::binary( + BinaryOp::Mul { unchecked: false }, + value, + casted_condition, + ), call_stack, ); Instruction::RangeCheck { value, max_bit_size, assert_message } @@ -718,8 +733,14 @@ impl<'f> Context<'f> { let cast = Instruction::Cast(condition, argument_type.unwrap_numeric()); let casted_condition = self.insert_instruction(cast, call_stack); + + // TODO: should this be unchecked? let field = self.insert_instruction( - Instruction::binary(BinaryOp::Mul, field, casted_condition), + Instruction::binary( + BinaryOp::Mul { unchecked: false }, + field, + casted_condition, + ), call_stack, ); @@ -797,11 +818,15 @@ impl<'f> Context<'f> { // Computes: if condition { var } else { 1 } fn var_or_one(&mut self, var: ValueId, condition: ValueId, call_stack: CallStackId) -> ValueId { - let field = - self.insert_instruction(Instruction::binary(BinaryOp::Mul, var, condition), call_stack); + // TODO: should this be unchecked? + let field = self.insert_instruction( + Instruction::binary(BinaryOp::Mul { unchecked: false }, var, condition), + call_stack, + ); let not_condition = self.not_instruction(condition, call_stack); + // TODO: should this be unchecked? self.insert_instruction( - Instruction::binary(BinaryOp::Add, field, not_condition), + Instruction::binary(BinaryOp::Add { unchecked: false }, field, not_condition), call_stack, ) } diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs index df351d6c0cd..4ae0ba544a4 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs @@ -121,13 +121,18 @@ impl<'a> ValueMerger<'a> { let else_condition = dfg.insert_instruction_and_results(cast, block, None, call_stack).first(); - let mul = Instruction::binary(BinaryOp::Mul, then_condition, then_value); + // TODO: should this be unchecked? + let mul = + Instruction::binary(BinaryOp::Mul { unchecked: false }, then_condition, then_value); let then_value = dfg.insert_instruction_and_results(mul, block, None, call_stack).first(); - let mul = Instruction::binary(BinaryOp::Mul, else_condition, else_value); + // TODO: should this be unchecked? + let mul = + Instruction::binary(BinaryOp::Mul { unchecked: false }, else_condition, else_value); let else_value = dfg.insert_instruction_and_results(mul, block, None, call_stack).first(); - let add = Instruction::binary(BinaryOp::Add, then_value, else_value); + // TODO: should this be unchecked? + let add = Instruction::binary(BinaryOp::Add { unchecked: false }, then_value, else_value); dfg.insert_instruction_and_results(add, block, None, call_stack).first() } diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index 36955480728..990e8954b17 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -936,7 +936,8 @@ mod test { // Compiling square f1 builder.new_function("square".into(), square_id, InlineType::default()); let square_v0 = builder.add_parameter(Type::field()); - let square_v2 = builder.insert_binary(square_v0, BinaryOp::Mul, square_v0); + let square_v2 = + builder.insert_binary(square_v0, BinaryOp::Mul { unchecked: false }, square_v0); builder.terminate_with_return(vec![square_v2]); // Compiling id1 f2 @@ -1001,9 +1002,9 @@ mod test { builder.switch_to_block(b2); let factorial_id = builder.import_function(factorial_id); - let v2 = builder.insert_binary(v0, BinaryOp::Sub, one); + let v2 = builder.insert_binary(v0, BinaryOp::Sub { unchecked: false }, one); let v3 = builder.insert_call(factorial_id, vec![v2], vec![Type::field()])[0]; - let v4 = builder.insert_binary(v0, BinaryOp::Mul, v3); + let v4 = builder.insert_binary(v0, BinaryOp::Mul { unchecked: false }, v3); builder.terminate_with_return(vec![v4]); let ssa = builder.finish(); diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index 09083369d4c..f0f677e5db9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -233,10 +233,7 @@ impl<'f> LoopInvariantContext<'f> { } } Instruction::Binary(binary) => { - if !matches!( - binary.operator, - BinaryOp::Add | BinaryOp::UncheckedAdd | BinaryOp::Mul | BinaryOp::UncheckedMul - ) { + if !matches!(binary.operator, BinaryOp::Add { .. } | BinaryOp::Mul { .. }) { return false; } diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 29ddfff323d..96a24583837 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -1002,7 +1002,7 @@ mod tests { let two = builder.field_constant(2u128); builder.insert_store(v5, two); let one = builder.field_constant(1u128); - let v3_plus_one = builder.insert_binary(v3, BinaryOp::Add, one); + let v3_plus_one = builder.insert_binary(v3, BinaryOp::Add { unchecked: false }, one); builder.terminate_with_jmp(b1, vec![v3_plus_one]); builder.switch_to_block(b3); diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs index 0af8fbb0b5e..980fab05d94 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs @@ -133,15 +133,23 @@ impl Context<'_> { let predicate = self.insert_cast(overflow, typ); let pow = self.pow(base, rhs); let pow = self.insert_cast(pow, typ); - (FieldElement::max_num_bits(), self.insert_binary(predicate, BinaryOp::Mul, pow)) + + // TODO: should this be unchecked? + ( + FieldElement::max_num_bits(), + self.insert_binary(predicate, BinaryOp::Mul { unchecked: false }, pow), + ) }; if max_bit <= bit_size { - self.insert_binary(lhs, BinaryOp::Mul, pow) + // TODO: should this be unchecked? + self.insert_binary(lhs, BinaryOp::Mul { unchecked: false }, pow) } else { let lhs_field = self.insert_cast(lhs, NumericType::NativeField); let pow_field = self.insert_cast(pow, NumericType::NativeField); - let result = self.insert_binary(lhs_field, BinaryOp::Mul, pow_field); + // TODO: should this be unchecked? + let result = + self.insert_binary(lhs_field, BinaryOp::Mul { unchecked: false }, pow_field); let result = self.insert_truncate(result, bit_size, max_bit); self.insert_cast(result, typ) } @@ -169,14 +177,24 @@ impl Context<'_> { let lhs_sign_as_field = self.insert_cast(lhs_sign, NumericType::NativeField); let lhs_as_field = self.insert_cast(lhs, NumericType::NativeField); // For negative numbers, convert to 1-complement using wrapping addition of a + 1 - let one_complement = self.insert_binary(lhs_sign_as_field, BinaryOp::Add, lhs_as_field); + // TODO: should this be unchecked? + let one_complement = self.insert_binary( + lhs_sign_as_field, + BinaryOp::Add { unchecked: false }, + lhs_as_field, + ); let one_complement = self.insert_truncate(one_complement, bit_size, bit_size + 1); let one_complement = self.insert_cast(one_complement, NumericType::signed(bit_size)); // Performs the division on the 1-complement (or the operand if positive) let shifted_complement = self.insert_binary(one_complement, BinaryOp::Div, pow); // Convert back to 2-complement representation if operand is negative let lhs_sign_as_int = self.insert_cast(lhs_sign, lhs_typ); - let shifted = self.insert_binary(shifted_complement, BinaryOp::Sub, lhs_sign_as_int); + // TODO: should this be unchecked? + let shifted = self.insert_binary( + shifted_complement, + BinaryOp::Sub { unchecked: false }, + lhs_sign_as_int, + ); self.insert_truncate(shifted, bit_size, bit_size + 1) } } @@ -201,16 +219,21 @@ impl Context<'_> { let one = self.field_constant(FieldElement::one()); let mut r = one; for i in 1..bit_size + 1 { - let r_squared = self.insert_binary(r, BinaryOp::Mul, r); - let a = self.insert_binary(r_squared, BinaryOp::Mul, lhs); + // TODO: should this be unchecked? + let r_squared = self.insert_binary(r, BinaryOp::Mul { unchecked: false }, r); + // TODO: should this be unchecked? + let a = self.insert_binary(r_squared, BinaryOp::Mul { unchecked: false }, lhs); let idx = self.field_constant(FieldElement::from((bit_size - i) as i128)); let b = self.insert_array_get(rhs_bits, idx, Type::bool()); let not_b = self.insert_not(b); let b = self.insert_cast(b, NumericType::NativeField); let not_b = self.insert_cast(not_b, NumericType::NativeField); - let r1 = self.insert_binary(a, BinaryOp::Mul, b); - let r2 = self.insert_binary(r_squared, BinaryOp::Mul, not_b); - r = self.insert_binary(r1, BinaryOp::Add, r2); + // TODO: should this be unchecked? + let r1 = self.insert_binary(a, BinaryOp::Mul { unchecked: false }, b); + // TODO: should this be unchecked? + let r2 = self.insert_binary(r_squared, BinaryOp::Mul { unchecked: false }, not_b); + // TODO: should this be unchecked? + r = self.insert_binary(r1, BinaryOp::Add { unchecked: false }, r2); } r } else { diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs index efe30748d49..ca9b75643bc 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs @@ -126,12 +126,9 @@ impl Context { use Instruction::*; match instruction { Binary(binary) => match binary.operator { - BinaryOp::Add - | BinaryOp::UncheckedAdd - | BinaryOp::Sub - | BinaryOp::UncheckedSub - | BinaryOp::Mul - | BinaryOp::UncheckedMul => dfg.type_of_value(binary.lhs).is_unsigned(), + BinaryOp::Add { .. } | BinaryOp::Sub { .. } | BinaryOp::Mul { .. } => { + dfg.type_of_value(binary.lhs).is_unsigned() + } BinaryOp::Div | BinaryOp::Mod => { if let Some(rhs) = dfg.get_numeric_constant(binary.rhs) { rhs == FieldElement::zero() @@ -242,13 +239,13 @@ mod test { let one = builder.numeric_constant(1u128, NumericType::bool()); builder.insert_enable_side_effects_if(one); - builder.insert_binary(v0, BinaryOp::Mul, two); + builder.insert_binary(v0, BinaryOp::Mul { unchecked: false }, two); builder.insert_enable_side_effects_if(one); - builder.insert_binary(v0, BinaryOp::Mul, two); + builder.insert_binary(v0, BinaryOp::Mul { unchecked: false }, two); builder.insert_enable_side_effects_if(one); - builder.insert_binary(v0, BinaryOp::Mul, two); + builder.insert_binary(v0, BinaryOp::Mul { unchecked: false }, two); builder.insert_enable_side_effects_if(one); - builder.insert_binary(v0, BinaryOp::Mul, two); + builder.insert_binary(v0, BinaryOp::Mul { unchecked: false }, two); builder.insert_enable_side_effects_if(one); let ssa = builder.finish(); @@ -278,7 +275,10 @@ mod test { assert_eq!(instructions.len(), 4); for instruction in instructions.iter().take(4) { - assert_eq!(&main.dfg[*instruction], &Instruction::binary(BinaryOp::Mul, v0, two)); + assert_eq!( + &main.dfg[*instruction], + &Instruction::binary(BinaryOp::Mul { unchecked: false }, v0, two) + ); } } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index 1caa5d14ce2..79181b7e74e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -624,8 +624,7 @@ impl Loop { .filter(|instruction| { let instruction = &function.dfg[**instruction]; matches!(instruction, - Instruction::Binary(Binary { lhs, operator: BinaryOp::Add, rhs: _ }) | - Instruction::Binary(Binary { lhs, operator: BinaryOp::UncheckedAdd, rhs: _ }) + Instruction::Binary(Binary { lhs, operator: BinaryOp::Add { .. }, rhs: _ }) if *lhs == induction_var ) }) diff --git a/compiler/noirc_evaluator/src/ssa/parser/mod.rs b/compiler/noirc_evaluator/src/ssa/parser/mod.rs index 6fc208ce41c..143ba511879 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/mod.rs @@ -286,9 +286,9 @@ impl<'a> Parser<'a> { fn eat_binary_op(&mut self) -> ParseResult> { let op = match self.token.token() { - Token::Keyword(Keyword::Add) => BinaryOp::Add, - Token::Keyword(Keyword::Sub) => BinaryOp::Sub, - Token::Keyword(Keyword::Mul) => BinaryOp::Mul, + Token::Keyword(Keyword::Add) => BinaryOp::Add { unchecked: false }, + Token::Keyword(Keyword::Sub) => BinaryOp::Sub { unchecked: false }, + Token::Keyword(Keyword::Mul) => BinaryOp::Mul { unchecked: false }, Token::Keyword(Keyword::Div) => BinaryOp::Div, Token::Keyword(Keyword::Eq) => BinaryOp::Eq, Token::Keyword(Keyword::Mod) => BinaryOp::Mod, @@ -298,9 +298,9 @@ impl<'a> Parser<'a> { Token::Keyword(Keyword::Xor) => BinaryOp::Xor, Token::Keyword(Keyword::Shl) => BinaryOp::Shl, Token::Keyword(Keyword::Shr) => BinaryOp::Shr, - Token::Keyword(Keyword::UncheckedAdd) => BinaryOp::UncheckedAdd, - Token::Keyword(Keyword::UncheckedSub) => BinaryOp::UncheckedSub, - Token::Keyword(Keyword::UncheckedMul) => BinaryOp::UncheckedMul, + Token::Keyword(Keyword::UncheckedAdd) => BinaryOp::Add { unchecked: true }, + Token::Keyword(Keyword::UncheckedSub) => BinaryOp::Sub { unchecked: true }, + Token::Keyword(Keyword::UncheckedMul) => BinaryOp::Mul { unchecked: true }, _ => return Ok(None), }; diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 9c3f3814b02..12ca9aeb5e2 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -317,12 +317,25 @@ impl<'a> FunctionContext<'a> { // We use unsafe casts here, this is fine as we're casting to a `field` type. let as_field = self.builder.insert_cast(input, NumericType::NativeField); let sign_field = self.builder.insert_cast(sign, NumericType::NativeField); - let positive_predicate = self.builder.insert_binary(sign_field, BinaryOp::Mul, as_field); - let two_complement = self.builder.insert_binary(bit_width, BinaryOp::Sub, as_field); + // TODO: should this be unchecked? + let positive_predicate = + self.builder.insert_binary(sign_field, BinaryOp::Mul { unchecked: false }, as_field); + // TODO: should this be unchecked? + let two_complement = + self.builder.insert_binary(bit_width, BinaryOp::Sub { unchecked: false }, as_field); let sign_not_field = self.builder.insert_cast(sign_not, NumericType::NativeField); - let negative_predicate = - self.builder.insert_binary(sign_not_field, BinaryOp::Mul, two_complement); - self.builder.insert_binary(positive_predicate, BinaryOp::Add, negative_predicate) + // TODO: should this be unchecked? + let negative_predicate = self.builder.insert_binary( + sign_not_field, + BinaryOp::Mul { unchecked: false }, + two_complement, + ); + // TODO: should this be unchecked? + self.builder.insert_binary( + positive_predicate, + BinaryOp::Add { unchecked: false }, + negative_predicate, + ) } /// Insert constraints ensuring that the operation does not overflow the bit size of the result @@ -477,8 +490,12 @@ impl<'a> FunctionContext<'a> { //Check the result has the same sign as its inputs let result_sign = self.builder.insert_binary(result, BinaryOp::Lt, half_width); let sign_diff = self.builder.insert_binary(result_sign, BinaryOp::Eq, lhs_sign); - let sign_diff_with_predicate = - self.builder.insert_binary(sign_diff, BinaryOp::Mul, same_sign); + // TODO: should this be unchecked? + let sign_diff_with_predicate = self.builder.insert_binary( + sign_diff, + BinaryOp::Mul { unchecked: false }, + same_sign, + ); let overflow_check = Instruction::Constrain( sign_diff_with_predicate, same_sign, @@ -491,7 +508,12 @@ impl<'a> FunctionContext<'a> { // First we compute the absolute value of operands, and their product let lhs_abs = self.absolute_value_helper(lhs, lhs_sign, bit_size); let rhs_abs = self.absolute_value_helper(rhs, rhs_sign, bit_size); - let product_field = self.builder.insert_binary(lhs_abs, BinaryOp::Mul, rhs_abs); + // TODO: should this be unchecked? + let product_field = self.builder.insert_binary( + lhs_abs, + BinaryOp::Mul { unchecked: false }, + rhs_abs, + ); // It must not already overflow the bit_size self.builder.set_location(location).insert_range_check( product_field, @@ -505,8 +527,12 @@ impl<'a> FunctionContext<'a> { let not_same = self.builder.insert_not(same_sign); let not_same_sign_field = self.insert_safe_cast(not_same, NumericType::unsigned(bit_size), location); - let positive_maximum_with_offset = - self.builder.insert_binary(half_width, BinaryOp::Add, not_same_sign_field); + // TODO: should this be unchecked + let positive_maximum_with_offset = self.builder.insert_binary( + half_width, + BinaryOp::Add { unchecked: false }, + not_same_sign_field, + ); let product_overflow_check = self.builder.insert_binary(product, BinaryOp::Lt, positive_maximum_with_offset); @@ -610,7 +636,8 @@ impl<'a> FunctionContext<'a> { if offset != 0 { let typ = self.builder.type_of_value(address).unwrap_numeric(); let offset = self.builder.numeric_constant(offset, typ); - address = self.builder.insert_binary(address, BinaryOp::UncheckedAdd, offset); + address = + self.builder.insert_binary(address, BinaryOp::Add { unchecked: true }, offset); } address } @@ -868,14 +895,19 @@ impl<'a> FunctionContext<'a> { self.builder.numeric_constant(self.element_size(array), NumericType::length_type()); // The actual base index is the user's index * the array element type's size - let mut index = - self.builder.set_location(location).insert_binary(index, BinaryOp::Mul, element_size); + // TODO: should this be unchecked? + let mut index = self.builder.set_location(location).insert_binary( + index, + BinaryOp::Mul { unchecked: false }, + element_size, + ); let one = self.builder.numeric_constant(FieldElement::one(), NumericType::length_type()); new_value.for_each(|value| { let value = value.eval(self); array = self.builder.insert_array_set(array, index, value); - index = self.builder.insert_binary(index, BinaryOp::Add, one); + // TODO: should this be unchecked? + index = self.builder.insert_binary(index, BinaryOp::Add { unchecked: false }, one); }); array } @@ -1000,9 +1032,9 @@ fn operator_requires_swapped_operands(op: BinaryOpKind) -> bool { /// to represent the full operation correctly. fn convert_operator(op: BinaryOpKind) -> BinaryOp { match op { - BinaryOpKind::Add => BinaryOp::Add, - BinaryOpKind::Subtract => BinaryOp::Sub, - BinaryOpKind::Multiply => BinaryOp::Mul, + BinaryOpKind::Add => BinaryOp::Add { unchecked: false }, + BinaryOpKind::Subtract => BinaryOp::Sub { unchecked: false }, + BinaryOpKind::Multiply => BinaryOp::Mul { unchecked: false }, BinaryOpKind::Divide => BinaryOp::Div, BinaryOpKind::Modulo => BinaryOp::Mod, BinaryOpKind::Equal => BinaryOp::Eq, diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 10b8129e8db..d68dab9a0b2 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -446,8 +446,12 @@ impl<'a> FunctionContext<'a> { let type_size = Self::convert_type(element_type).size_of_type(); let type_size = self.builder.numeric_constant(type_size as u128, NumericType::length_type()); - let base_index = - self.builder.set_location(location).insert_binary(index, BinaryOp::Mul, type_size); + // TODO: should this be unchecked? + let base_index = self.builder.set_location(location).insert_binary( + index, + BinaryOp::Mul { unchecked: false }, + type_size, + ); let mut field_index = 0u128; Ok(Self::map_type(element_type, |typ| { @@ -702,7 +706,12 @@ impl<'a> FunctionContext<'a> { // We add one here in the case of a slice insert as a slice insert at the length of the slice // can be converted to a slice push back - let len_plus_one = self.builder.insert_binary(arguments[0], BinaryOp::Add, one); + // TODO: should this be unchecked? + let len_plus_one = self.builder.insert_binary( + arguments[0], + BinaryOp::Add { unchecked: false }, + one, + ); self.codegen_slice_access_check(arguments[2], Some(len_plus_one)); } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs index 98cdc0adad9..068160bd7f9 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs @@ -129,8 +129,8 @@ mod test { let one = builder.field_constant(1u128); let three = builder.field_constant(3u128); - let v1 = builder.insert_binary(v0, BinaryOp::Add, one); - let v2 = builder.insert_binary(v1, BinaryOp::Mul, three); + let v1 = builder.insert_binary(v0, BinaryOp::Add { unchecked: false }, one); + let v2 = builder.insert_binary(v1, BinaryOp::Mul { unchecked: false }, three); builder.terminate_with_return(vec![v2]); let ssa = builder.finish(); From 84de8544d2d056ebddb50e874ad077fcc0dfa6a4 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 9 Jan 2025 17:34:37 -0300 Subject: [PATCH 09/24] increment and decrement slice length --- .../src/ssa/ir/instruction/call.rs | 72 +++++++------------ 1 file changed, 25 insertions(+), 47 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 63c2434c8aa..8bb14e12085 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -142,13 +142,7 @@ pub(super) fn simplify_call( slice.push_back(*elem); } - // TODO: should this use an unchecked add? - let new_slice_length = update_slice_length( - arguments[0], - dfg, - BinaryOp::Add { unchecked: false }, - block, - ); + let new_slice_length = increment_slice_length(arguments[0], dfg, block); let new_slice = make_array(dfg, slice, element_type, block, call_stack); return SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]); @@ -166,13 +160,7 @@ pub(super) fn simplify_call( slice.push_front(*elem); } - // TODO: should this be unchecked? - let new_slice_length = update_slice_length( - arguments[0], - dfg, - BinaryOp::Add { unchecked: false }, - block, - ); + let new_slice_length = increment_slice_length(arguments[0], dfg, block); let new_slice = make_array(dfg, slice, element_type, block, call_stack); SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) @@ -212,13 +200,7 @@ pub(super) fn simplify_call( slice.pop_front().expect("There are no elements in this slice to be removed") }); - // TODO: should this be unchecked? - let new_slice_length = update_slice_length( - arguments[0], - dfg, - BinaryOp::Sub { unchecked: false }, - block, - ); + let new_slice_length = decrement_slice_length(arguments[0], dfg, block); results.push(new_slice_length); @@ -251,13 +233,7 @@ pub(super) fn simplify_call( index += 1; } - // TODO: should this be unchecked? - let new_slice_length = update_slice_length( - arguments[0], - dfg, - BinaryOp::Add { unchecked: false }, - block, - ); + let new_slice_length = increment_slice_length(arguments[0], dfg, block); let new_slice = make_array(dfg, slice, typ, block, call_stack); SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) @@ -295,13 +271,7 @@ pub(super) fn simplify_call( let new_slice = make_array(dfg, slice, typ, block, call_stack); results.insert(0, new_slice); - // TODO: should this be unchecked? - let new_slice_length = update_slice_length( - arguments[0], - dfg, - BinaryOp::Sub { unchecked: false }, - block, - ); + let new_slice_length = decrement_slice_length(arguments[0], dfg, block); results.insert(0, new_slice_length); @@ -417,6 +387,22 @@ fn update_slice_length( dfg.insert_instruction_and_results(instruction, block, None, call_stack).first() } +fn increment_slice_length( + slice_len: ValueId, + dfg: &mut DataFlowGraph, + block: BasicBlockId, +) -> ValueId { + update_slice_length(slice_len, dfg, BinaryOp::Add { unchecked: false }, block) +} + +fn decrement_slice_length( + slice_len: ValueId, + dfg: &mut DataFlowGraph, + block: BasicBlockId, +) -> ValueId { + update_slice_length(slice_len, dfg, BinaryOp::Sub { unchecked: true }, block) +} + fn simplify_slice_push_back( mut slice: im::Vector, element_type: Type, @@ -437,9 +423,7 @@ fn simplify_slice_push_back( .insert_instruction_and_results(len_not_equals_capacity_instr, block, None, call_stack) .first(); - // TODO: should this be unchecked? - let new_slice_length = - update_slice_length(arguments[0], dfg, BinaryOp::Add { unchecked: false }, block); + let new_slice_length = increment_slice_length(arguments[0], dfg, block); for elem in &arguments[2..] { slice.push_back(*elem); @@ -488,9 +472,7 @@ fn simplify_slice_pop_back( let element_count = element_types.len(); let mut results = VecDeque::with_capacity(element_count + 1); - // TODO: should this be unchecked? - let new_slice_length = - update_slice_length(arguments[0], dfg, BinaryOp::Sub { unchecked: false }, block); + let new_slice_length = decrement_slice_length(arguments[0], dfg, block); let element_size = dfg.make_constant((element_count as u128).into(), NumericType::length_type()); @@ -499,9 +481,7 @@ fn simplify_slice_pop_back( Instruction::binary(BinaryOp::Mul { unchecked: false }, arguments[0], element_size); let mut flattened_len = dfg.insert_instruction_and_results(flattened_len_instr, block, None, call_stack).first(); - // TODO: should this be unchecked? - flattened_len = - update_slice_length(flattened_len, dfg, BinaryOp::Sub { unchecked: false }, block); + flattened_len = decrement_slice_length(flattened_len, dfg, block); // We must pop multiple elements in the case of a slice of tuples // Iterating through element types in reverse here since we're popping from the end @@ -515,9 +495,7 @@ fn simplify_slice_pop_back( .first(); results.push_front(get_last_elem); - // TODO: should this be unchecked? - flattened_len = - update_slice_length(flattened_len, dfg, BinaryOp::Sub { unchecked: false }, block); + flattened_len = decrement_slice_length(flattened_len, dfg, block); } results.push_front(arguments[1]); From 7dff466b223c434b649d72499b79021070537b8e Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 10:31:06 -0300 Subject: [PATCH 10/24] Handle some TODOs --- .../src/ssa/ir/instruction/binary.rs | 4 +- .../src/ssa/ir/instruction/call.rs | 5 +- compiler/noirc_evaluator/src/ssa/opt/die.rs | 8 +- .../src/ssa/opt/flatten_cfg.rs | 95 ++++++++----------- .../src/ssa/opt/flatten_cfg/value_merger.rs | 12 +-- 5 files changed, 52 insertions(+), 72 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs index 7cd3a92966d..79817edb3e5 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs @@ -250,9 +250,9 @@ impl Binary { } if operand_type == NumericType::bool() { // Boolean AND is equivalent to multiplication, which is a cheaper operation. - // TODO: should this be unchecked? + // (mul unchecked because these are bools so it doesn't matter really) let instruction = - Instruction::binary(BinaryOp::Mul { unchecked: false }, self.lhs, self.rhs); + Instruction::binary(BinaryOp::Mul { unchecked: true }, self.lhs, self.rhs); return SimplifyResult::SimplifiedToInstruction(instruction); } if operand_type.is_unsigned() { diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 8bb14e12085..992c633ffcd 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -476,9 +476,10 @@ fn simplify_slice_pop_back( let element_size = dfg.make_constant((element_count as u128).into(), NumericType::length_type()); - // TODO: should this be unchecked? + // Compute the flattened length doing an unchecked mul + // (it shouldn't overflow because it would have overflowed before when the slice was created) let flattened_len_instr = - Instruction::binary(BinaryOp::Mul { unchecked: false }, arguments[0], element_size); + Instruction::binary(BinaryOp::Mul { unchecked: true }, arguments[0], element_size); let mut flattened_len = dfg.insert_instruction_and_results(flattened_len_instr, block, None, call_stack).first(); flattened_len = decrement_slice_length(flattened_len, dfg, block); diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index cda993deb37..eed1af8251b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -507,18 +507,18 @@ fn apply_side_effects( let casted_condition = dfg.insert_instruction_and_results(cast, block_id, None, call_stack); let casted_condition = casted_condition.first(); - // TODO: should this be unchecked? + // Unchecked mul because the side effects var is always 0 or 1 let lhs = dfg.insert_instruction_and_results( - Instruction::binary(BinaryOp::Mul { unchecked: false }, lhs, casted_condition), + Instruction::binary(BinaryOp::Mul { unchecked: true }, lhs, casted_condition), block_id, None, call_stack, ); let lhs = lhs.first(); - // TODO: should this be unchecked? + // Unchecked mul because the side effects var is always 0 or 1 let rhs = dfg.insert_instruction_and_results( - Instruction::binary(BinaryOp::Mul { unchecked: false }, rhs, casted_condition), + Instruction::binary(BinaryOp::Mul { unchecked: true }, rhs, casted_condition), block_id, None, call_stack, diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index b36844386f2..f9feaad033b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -654,30 +654,10 @@ impl<'f> Context<'f> { // Replace constraint `lhs == rhs` with `condition * lhs == condition * rhs`. // Condition needs to be cast to argument type in order to multiply them together. - let argument_type = self.inserter.function.dfg.type_of_value(lhs); - - let cast = Instruction::Cast(condition, argument_type.unwrap_numeric()); - let casted_condition = self.insert_instruction(cast, call_stack); - - // TODO: should this be unchecked? - let lhs = self.insert_instruction( - Instruction::binary( - BinaryOp::Mul { unchecked: false }, - lhs, - casted_condition, - ), - call_stack, - ); - // TODO: should this be unchecked? - let rhs = self.insert_instruction( - Instruction::binary( - BinaryOp::Mul { unchecked: false }, - rhs, - casted_condition, - ), - call_stack, - ); - + let casted_condition = + self.cast_condition_to_value_type(condition, lhs, call_stack); + let lhs = self.mul_by_condition(lhs, casted_condition, call_stack); + let rhs = self.mul_by_condition(rhs, casted_condition, call_stack); Instruction::Constrain(lhs, rhs, message) } Instruction::Store { address, value } => { @@ -710,39 +690,18 @@ impl<'f> Context<'f> { // Replace value with `value * predicate` to zero out value when predicate is inactive. // Condition needs to be cast to argument type in order to multiply them together. - let argument_type = self.inserter.function.dfg.type_of_value(value); - let cast = Instruction::Cast(condition, argument_type.unwrap_numeric()); - let casted_condition = self.insert_instruction(cast, call_stack); - - // TODO: should this be unchecked? - let value = self.insert_instruction( - Instruction::binary( - BinaryOp::Mul { unchecked: false }, - value, - casted_condition, - ), - call_stack, - ); + let casted_condition = + self.cast_condition_to_value_type(condition, value, call_stack); + let value = self.mul_by_condition(value, casted_condition, call_stack); Instruction::RangeCheck { value, max_bit_size, assert_message } } Instruction::Call { func, mut arguments } => match self.inserter.function.dfg[func] { Value::Intrinsic(Intrinsic::ToBits(_) | Intrinsic::ToRadix(_)) => { let field = arguments[0]; - let argument_type = self.inserter.function.dfg.type_of_value(field); - - let cast = Instruction::Cast(condition, argument_type.unwrap_numeric()); - let casted_condition = self.insert_instruction(cast, call_stack); - - // TODO: should this be unchecked? - let field = self.insert_instruction( - Instruction::binary( - BinaryOp::Mul { unchecked: false }, - field, - casted_condition, - ), - call_stack, - ); + let casted_condition = + self.cast_condition_to_value_type(condition, field, call_stack); + let field = self.mul_by_condition(field, casted_condition, call_stack); arguments[0] = field; @@ -786,6 +745,30 @@ impl<'f> Context<'f> { } } + fn cast_condition_to_value_type( + &mut self, + condition: ValueId, + value: ValueId, + call_stack: CallStackId, + ) -> ValueId { + let argument_type = self.inserter.function.dfg.type_of_value(value); + let cast = Instruction::Cast(condition, argument_type.unwrap_numeric()); + self.insert_instruction(cast, call_stack) + } + + fn mul_by_condition( + &mut self, + value: ValueId, + condition: ValueId, + call_stack: CallStackId, + ) -> ValueId { + // Unchecked mul because the condition is always 0 or 1 + self.insert_instruction( + Instruction::binary(BinaryOp::Mul { unchecked: true }, value, condition), + call_stack, + ) + } + /// When a MSM is done under a predicate, we need to apply the predicate /// to the is_infinity property of the input points in order to ensure /// that the points will be on the curve no matter what. @@ -818,15 +801,11 @@ impl<'f> Context<'f> { // Computes: if condition { var } else { 1 } fn var_or_one(&mut self, var: ValueId, condition: ValueId, call_stack: CallStackId) -> ValueId { - // TODO: should this be unchecked? - let field = self.insert_instruction( - Instruction::binary(BinaryOp::Mul { unchecked: false }, var, condition), - call_stack, - ); + let field = self.mul_by_condition(var, condition, call_stack); let not_condition = self.not_instruction(condition, call_stack); - // TODO: should this be unchecked? + // Unchecked add because of the values is guaranteed to be 0 self.insert_instruction( - Instruction::binary(BinaryOp::Add { unchecked: false }, field, not_condition), + Instruction::binary(BinaryOp::Add { unchecked: true }, field, not_condition), call_stack, ) } diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs index 4ae0ba544a4..f4638cf85e4 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs @@ -121,18 +121,18 @@ impl<'a> ValueMerger<'a> { let else_condition = dfg.insert_instruction_and_results(cast, block, None, call_stack).first(); - // TODO: should this be unchecked? + // Unchecked mul because `then_condition` will be 1 or 0 let mul = - Instruction::binary(BinaryOp::Mul { unchecked: false }, then_condition, then_value); + Instruction::binary(BinaryOp::Mul { unchecked: true }, then_condition, then_value); let then_value = dfg.insert_instruction_and_results(mul, block, None, call_stack).first(); - // TODO: should this be unchecked? + // Unchecked mul because `else_condition` will be 1 or 0 let mul = - Instruction::binary(BinaryOp::Mul { unchecked: false }, else_condition, else_value); + Instruction::binary(BinaryOp::Mul { unchecked: true }, else_condition, else_value); let else_value = dfg.insert_instruction_and_results(mul, block, None, call_stack).first(); - // TODO: should this be unchecked? - let add = Instruction::binary(BinaryOp::Add { unchecked: false }, then_value, else_value); + // Unchecked add because one of the values will always be 0 + let add = Instruction::binary(BinaryOp::Add { unchecked: true }, then_value, else_value); dfg.insert_instruction_and_results(add, block, None, call_stack).first() } From dfad378cdb5b5e29ded43ef6b628dc6cb6599769 Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 10 Jan 2025 13:46:42 +0000 Subject: [PATCH 11/24] chore: address some todos --- .../src/ssa/opt/remove_bit_shifts.rs | 18 +++++++----------- .../noirc_evaluator/src/ssa/ssa_gen/context.rs | 8 ++++---- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs index 980fab05d94..d1615c6d1aa 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs @@ -134,7 +134,7 @@ impl Context<'_> { let pow = self.pow(base, rhs); let pow = self.insert_cast(pow, typ); - // TODO: should this be unchecked? + // Unchecked mul because `predicate` will be 1 or 0 ( FieldElement::max_num_bits(), self.insert_binary(predicate, BinaryOp::Mul { unchecked: false }, pow), @@ -218,22 +218,18 @@ impl Context<'_> { let rhs_bits = rhs_bits[0]; let one = self.field_constant(FieldElement::one()); let mut r = one; + // All operations are unchecked as we're acting on Field types (which are always unchecked) for i in 1..bit_size + 1 { - // TODO: should this be unchecked? - let r_squared = self.insert_binary(r, BinaryOp::Mul { unchecked: false }, r); - // TODO: should this be unchecked? - let a = self.insert_binary(r_squared, BinaryOp::Mul { unchecked: false }, lhs); + let r_squared = self.insert_binary(r, BinaryOp::Mul { unchecked: true }, r); + let a = self.insert_binary(r_squared, BinaryOp::Mul { unchecked: true }, lhs); let idx = self.field_constant(FieldElement::from((bit_size - i) as i128)); let b = self.insert_array_get(rhs_bits, idx, Type::bool()); let not_b = self.insert_not(b); let b = self.insert_cast(b, NumericType::NativeField); let not_b = self.insert_cast(not_b, NumericType::NativeField); - // TODO: should this be unchecked? - let r1 = self.insert_binary(a, BinaryOp::Mul { unchecked: false }, b); - // TODO: should this be unchecked? - let r2 = self.insert_binary(r_squared, BinaryOp::Mul { unchecked: false }, not_b); - // TODO: should this be unchecked? - r = self.insert_binary(r1, BinaryOp::Add { unchecked: false }, r2); + let r1 = self.insert_binary(a, BinaryOp::Mul { unchecked: true }, b); + let r2 = self.insert_binary(r_squared, BinaryOp::Mul { unchecked: true }, not_b); + r = self.insert_binary(r1, BinaryOp::Add { unchecked: true }, r2); } r } else { diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 12ca9aeb5e2..c3ca161895c 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -330,7 +330,7 @@ impl<'a> FunctionContext<'a> { BinaryOp::Mul { unchecked: false }, two_complement, ); - // TODO: should this be unchecked? + // Unchecked addition because either `positive_predicate` or `negative_predicate` will be 0 self.builder.insert_binary( positive_predicate, BinaryOp::Add { unchecked: false }, @@ -490,10 +490,10 @@ impl<'a> FunctionContext<'a> { //Check the result has the same sign as its inputs let result_sign = self.builder.insert_binary(result, BinaryOp::Lt, half_width); let sign_diff = self.builder.insert_binary(result_sign, BinaryOp::Eq, lhs_sign); - // TODO: should this be unchecked? + // Unchecked multiplication because boolean inputs let sign_diff_with_predicate = self.builder.insert_binary( sign_diff, - BinaryOp::Mul { unchecked: false }, + BinaryOp::Mul { unchecked: true }, same_sign, ); let overflow_check = Instruction::Constrain( @@ -527,7 +527,7 @@ impl<'a> FunctionContext<'a> { let not_same = self.builder.insert_not(same_sign); let not_same_sign_field = self.insert_safe_cast(not_same, NumericType::unsigned(bit_size), location); - // TODO: should this be unchecked + // TODO: should this be unchecked? let positive_maximum_with_offset = self.builder.insert_binary( half_width, BinaryOp::Add { unchecked: false }, From 7840ced3b3bfd7f4f186fe5a1520c04ec34cf305 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 10:47:18 -0300 Subject: [PATCH 12/24] Handle more TODOs --- .../src/ssa/opt/remove_bit_shifts.rs | 36 ++++++++--------- .../src/ssa/ssa_gen/context.rs | 40 +++++++++---------- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 7 ++-- 3 files changed, 38 insertions(+), 45 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs index 980fab05d94..e8e23bbb798 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs @@ -134,22 +134,22 @@ impl Context<'_> { let pow = self.pow(base, rhs); let pow = self.insert_cast(pow, typ); - // TODO: should this be unchecked? + // Unchecked mul because predicate is 0 or 1 ( FieldElement::max_num_bits(), - self.insert_binary(predicate, BinaryOp::Mul { unchecked: false }, pow), + self.insert_binary(predicate, BinaryOp::Mul { unchecked: true }, pow), ) }; if max_bit <= bit_size { - // TODO: should this be unchecked? - self.insert_binary(lhs, BinaryOp::Mul { unchecked: false }, pow) + // Unchecked mul as it can't overflow + self.insert_binary(lhs, BinaryOp::Mul { unchecked: true }, pow) } else { let lhs_field = self.insert_cast(lhs, NumericType::NativeField); let pow_field = self.insert_cast(pow, NumericType::NativeField); - // TODO: should this be unchecked? + // Unchecked mul as this is a wrapping operation that we later truncate let result = - self.insert_binary(lhs_field, BinaryOp::Mul { unchecked: false }, pow_field); + self.insert_binary(lhs_field, BinaryOp::Mul { unchecked: true }, pow_field); let result = self.insert_truncate(result, bit_size, max_bit); self.insert_cast(result, typ) } @@ -177,10 +177,10 @@ impl Context<'_> { let lhs_sign_as_field = self.insert_cast(lhs_sign, NumericType::NativeField); let lhs_as_field = self.insert_cast(lhs, NumericType::NativeField); // For negative numbers, convert to 1-complement using wrapping addition of a + 1 - // TODO: should this be unchecked? + // Unchecked add as these are fields let one_complement = self.insert_binary( lhs_sign_as_field, - BinaryOp::Add { unchecked: false }, + BinaryOp::Add { unchecked: true }, lhs_as_field, ); let one_complement = self.insert_truncate(one_complement, bit_size, bit_size + 1); @@ -189,10 +189,10 @@ impl Context<'_> { let shifted_complement = self.insert_binary(one_complement, BinaryOp::Div, pow); // Convert back to 2-complement representation if operand is negative let lhs_sign_as_int = self.insert_cast(lhs_sign, lhs_typ); - // TODO: should this be unchecked? + // This shouldn't underflow let shifted = self.insert_binary( shifted_complement, - BinaryOp::Sub { unchecked: false }, + BinaryOp::Sub { unchecked: true }, lhs_sign_as_int, ); self.insert_truncate(shifted, bit_size, bit_size + 1) @@ -219,21 +219,17 @@ impl Context<'_> { let one = self.field_constant(FieldElement::one()); let mut r = one; for i in 1..bit_size + 1 { - // TODO: should this be unchecked? - let r_squared = self.insert_binary(r, BinaryOp::Mul { unchecked: false }, r); - // TODO: should this be unchecked? - let a = self.insert_binary(r_squared, BinaryOp::Mul { unchecked: false }, lhs); + // All of these operations are unchecked because they deal with fields + let r_squared = self.insert_binary(r, BinaryOp::Mul { unchecked: true }, r); + let a = self.insert_binary(r_squared, BinaryOp::Mul { unchecked: true }, lhs); let idx = self.field_constant(FieldElement::from((bit_size - i) as i128)); let b = self.insert_array_get(rhs_bits, idx, Type::bool()); let not_b = self.insert_not(b); let b = self.insert_cast(b, NumericType::NativeField); let not_b = self.insert_cast(not_b, NumericType::NativeField); - // TODO: should this be unchecked? - let r1 = self.insert_binary(a, BinaryOp::Mul { unchecked: false }, b); - // TODO: should this be unchecked? - let r2 = self.insert_binary(r_squared, BinaryOp::Mul { unchecked: false }, not_b); - // TODO: should this be unchecked? - r = self.insert_binary(r1, BinaryOp::Add { unchecked: false }, r2); + let r1 = self.insert_binary(a, BinaryOp::Mul { unchecked: true }, b); + let r2 = self.insert_binary(r_squared, BinaryOp::Mul { unchecked: true }, not_b); + r = self.insert_binary(r1, BinaryOp::Add { unchecked: true }, r2); } r } else { diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 12ca9aeb5e2..d9da8c94d98 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -317,23 +317,21 @@ impl<'a> FunctionContext<'a> { // We use unsafe casts here, this is fine as we're casting to a `field` type. let as_field = self.builder.insert_cast(input, NumericType::NativeField); let sign_field = self.builder.insert_cast(sign, NumericType::NativeField); - // TODO: should this be unchecked? + + // All of these operations are unchecked because they deal with fields let positive_predicate = - self.builder.insert_binary(sign_field, BinaryOp::Mul { unchecked: false }, as_field); - // TODO: should this be unchecked? + self.builder.insert_binary(sign_field, BinaryOp::Mul { unchecked: true }, as_field); let two_complement = - self.builder.insert_binary(bit_width, BinaryOp::Sub { unchecked: false }, as_field); + self.builder.insert_binary(bit_width, BinaryOp::Sub { unchecked: true }, as_field); let sign_not_field = self.builder.insert_cast(sign_not, NumericType::NativeField); - // TODO: should this be unchecked? let negative_predicate = self.builder.insert_binary( sign_not_field, - BinaryOp::Mul { unchecked: false }, + BinaryOp::Mul { unchecked: true }, two_complement, ); - // TODO: should this be unchecked? self.builder.insert_binary( positive_predicate, - BinaryOp::Add { unchecked: false }, + BinaryOp::Add { unchecked: true }, negative_predicate, ) } @@ -490,10 +488,10 @@ impl<'a> FunctionContext<'a> { //Check the result has the same sign as its inputs let result_sign = self.builder.insert_binary(result, BinaryOp::Lt, half_width); let sign_diff = self.builder.insert_binary(result_sign, BinaryOp::Eq, lhs_sign); - // TODO: should this be unchecked? + // Unchecked mul because sign_diff is a boolean let sign_diff_with_predicate = self.builder.insert_binary( sign_diff, - BinaryOp::Mul { unchecked: false }, + BinaryOp::Mul { unchecked: true }, same_sign, ); let overflow_check = Instruction::Constrain( @@ -508,12 +506,9 @@ impl<'a> FunctionContext<'a> { // First we compute the absolute value of operands, and their product let lhs_abs = self.absolute_value_helper(lhs, lhs_sign, bit_size); let rhs_abs = self.absolute_value_helper(rhs, rhs_sign, bit_size); - // TODO: should this be unchecked? - let product_field = self.builder.insert_binary( - lhs_abs, - BinaryOp::Mul { unchecked: false }, - rhs_abs, - ); + // Unchecked mul because these are fields + let product_field = + self.builder.insert_binary(lhs_abs, BinaryOp::Mul { unchecked: true }, rhs_abs); // It must not already overflow the bit_size self.builder.set_location(location).insert_range_check( product_field, @@ -527,10 +522,10 @@ impl<'a> FunctionContext<'a> { let not_same = self.builder.insert_not(same_sign); let not_same_sign_field = self.insert_safe_cast(not_same, NumericType::unsigned(bit_size), location); - // TODO: should this be unchecked + // This shouldn't overflow so it's unchecked let positive_maximum_with_offset = self.builder.insert_binary( half_width, - BinaryOp::Add { unchecked: false }, + BinaryOp::Add { unchecked: true }, not_same_sign_field, ); let product_overflow_check = @@ -895,10 +890,11 @@ impl<'a> FunctionContext<'a> { self.builder.numeric_constant(self.element_size(array), NumericType::length_type()); // The actual base index is the user's index * the array element type's size - // TODO: should this be unchecked? + // Unchecked mul because we are reaching for an array element: if it overflows here + // it would have overflowed when creating the array. let mut index = self.builder.set_location(location).insert_binary( index, - BinaryOp::Mul { unchecked: false }, + BinaryOp::Mul { unchecked: true }, element_size, ); let one = self.builder.numeric_constant(FieldElement::one(), NumericType::length_type()); @@ -906,8 +902,8 @@ impl<'a> FunctionContext<'a> { new_value.for_each(|value| { let value = value.eval(self); array = self.builder.insert_array_set(array, index, value); - // TODO: should this be unchecked? - index = self.builder.insert_binary(index, BinaryOp::Add { unchecked: false }, one); + // Unchecked add because this can't overflow (it would have overflowed when creating the array) + index = self.builder.insert_binary(index, BinaryOp::Add { unchecked: true }, one); }); array } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index d68dab9a0b2..28ff544b857 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -446,10 +446,11 @@ impl<'a> FunctionContext<'a> { let type_size = Self::convert_type(element_type).size_of_type(); let type_size = self.builder.numeric_constant(type_size as u128, NumericType::length_type()); - // TODO: should this be unchecked? + // This shouldn't overflow as we are reaching for an initial array offset + // (otherwise it would have overflowed when creating the array) let base_index = self.builder.set_location(location).insert_binary( index, - BinaryOp::Mul { unchecked: false }, + BinaryOp::Mul { unchecked: true }, type_size, ); @@ -706,7 +707,7 @@ impl<'a> FunctionContext<'a> { // We add one here in the case of a slice insert as a slice insert at the length of the slice // can be converted to a slice push back - // TODO: should this be unchecked? + // This is unchecked as the slice length could be u32::max let len_plus_one = self.builder.insert_binary( arguments[0], BinaryOp::Add { unchecked: false }, From 0df89c00396fc01c83b1fb7d03796ebd3b33021c Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 10:55:26 -0300 Subject: [PATCH 13/24] Handle one more TODO --- compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs | 4 ++-- compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs index 6cc5239e7a8..180f91acf0c 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs @@ -189,10 +189,10 @@ impl Context<'_> { let shifted_complement = self.insert_binary(one_complement, BinaryOp::Div, pow); // Convert back to 2-complement representation if operand is negative let lhs_sign_as_int = self.insert_cast(lhs_sign, lhs_typ); - // This shouldn't underflow + // TODO: should this be unchecked? let shifted = self.insert_binary( shifted_complement, - BinaryOp::Sub { unchecked: true }, + BinaryOp::Sub { unchecked: false }, lhs_sign_as_int, ); self.insert_truncate(shifted, bit_size, bit_size + 1) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index f54abf47604..23166386ddb 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -523,7 +523,7 @@ impl<'a> FunctionContext<'a> { let not_same = self.builder.insert_not(same_sign); let not_same_sign_field = self.insert_safe_cast(not_same, NumericType::unsigned(bit_size), location); - // TODO: should this be unchecked? + // Unchecked add because adding 1 to half_width can't overflow let positive_maximum_with_offset = self.builder.insert_binary( half_width, BinaryOp::Add { unchecked: true }, From 6f9d1c723b83462b78c1b1bb7cea5a6dca9352a5 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 11:11:28 -0300 Subject: [PATCH 14/24] Fix tests --- .../src/ssa/opt/flatten_cfg.rs | 68 +++++++++---------- 1 file changed, 34 insertions(+), 34 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index f9feaad033b..795e06bceb9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -850,9 +850,9 @@ mod test { enable_side_effects u1 1 v3 = cast v0 as Field v4 = cast v1 as Field - v6 = mul v3, Field 3 - v8 = mul v4, Field 4 - v9 = add v6, v8 + v6 = unchecked_mul v3, Field 3 + v8 = unchecked_mul v4, Field 4 + v9 = unchecked_add v6, v8 return v9 } "; @@ -881,7 +881,7 @@ mod test { acir(inline) fn main f0 { b0(v0: u1, v1: u1): enable_side_effects v0 - v2 = mul v1, v0 + v2 = unchecked_mul v1, v0 constrain v2 == v0 v3 = not v0 enable_side_effects u1 1 @@ -916,9 +916,9 @@ mod test { v3 = not v0 v4 = cast v0 as Field v5 = cast v3 as Field - v7 = mul v4, Field 5 - v8 = mul v5, v2 - v9 = add v7, v8 + v7 = unchecked_mul v4, Field 5 + v8 = unchecked_mul v5, v2 + v9 = unchecked_add v7, v8 store v9 at v1 enable_side_effects u1 1 return @@ -954,17 +954,17 @@ mod test { v3 = not v0 v4 = cast v0 as Field v5 = cast v3 as Field - v7 = mul v4, Field 5 - v8 = mul v5, v2 - v9 = add v7, v8 + v7 = unchecked_mul v4, Field 5 + v8 = unchecked_mul v5, v2 + v9 = unchecked_add v7, v8 store v9 at v1 enable_side_effects v3 v10 = load v1 -> Field v11 = cast v3 as Field v12 = cast v0 as Field - v14 = mul v11, Field 6 - v15 = mul v12, v10 - v16 = add v14, v15 + v14 = unchecked_mul v11, Field 6 + v15 = unchecked_mul v12, v10 + v16 = unchecked_add v14, v15 store v16 at v1 enable_side_effects u1 1 return @@ -1066,32 +1066,32 @@ mod test { v3 = not v0 v4 = cast v0 as Field v5 = cast v3 as Field - v7 = mul v4, Field 2 - v8 = add v7, v5 - v9 = mul v0, v1 + v7 = unchecked_mul v4, Field 2 + v8 = unchecked_add v7, v5 + v9 = unchecked_mul v0, v1 enable_side_effects v9 v10 = not v9 v11 = cast v9 as Field v12 = cast v10 as Field - v14 = mul v11, Field 5 - v15 = mul v12, v8 - v16 = add v14, v15 + v14 = unchecked_mul v11, Field 5 + v15 = unchecked_mul v12, v8 + v16 = unchecked_add v14, v15 v17 = not v1 - v18 = mul v0, v17 + v18 = unchecked_mul v0, v17 enable_side_effects v18 v19 = not v18 v20 = cast v18 as Field v21 = cast v19 as Field - v23 = mul v20, Field 6 - v24 = mul v21, v16 - v25 = add v23, v24 + v23 = unchecked_mul v20, Field 6 + v24 = unchecked_mul v21, v16 + v25 = unchecked_add v23, v24 enable_side_effects v0 enable_side_effects v3 v26 = cast v3 as Field v27 = cast v0 as Field - v29 = mul v26, Field 3 - v30 = mul v27, v25 - v31 = add v29, v30 + v29 = unchecked_mul v26, Field 3 + v30 = unchecked_mul v27, v25 + v31 = unchecked_add v29, v30 enable_side_effects u1 1 return v31 }"; @@ -1283,15 +1283,15 @@ mod test { v12 = not v5 v13 = cast v4 as u8 v14 = cast v12 as u8 - v15 = mul v13, v10 - v16 = mul v14, v11 - v17 = add v15, v16 + v15 = unchecked_mul v13, v10 + v16 = unchecked_mul v14, v11 + v17 = unchecked_add v15, v16 store v17 at v6 enable_side_effects v12 v18 = load v6 -> u8 v19 = cast v12 as u8 v20 = cast v4 as u8 - v21 = mul v20, v18 + v21 = unchecked_mul v20, v18 store v21 at v6 enable_side_effects u1 1 constrain v5 == u1 1 @@ -1446,9 +1446,9 @@ mod test { v2 = not v0 v3 = cast v0 as Field v4 = cast v2 as Field - v6 = mul v3, Field 2 - v7 = mul v4, v3 - v8 = add v6, v7 + v6 = unchecked_mul v3, Field 2 + v7 = unchecked_mul v4, v3 + v8 = unchecked_add v6, v7 enable_side_effects u1 1 return v8 } @@ -1491,7 +1491,7 @@ mod test { b0(v0: u1, v1: u1): enable_side_effects v0 v2 = cast v0 as Field - v4 = mul v2, Field 2 + v4 = unchecked_mul v2, Field 2 v5 = make_array [v4] : [Field; 1] enable_side_effects u1 1 return v5 From f34fb51e4ae13cc8c77c0ddf391ae3a9a38b4b81 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 11:19:50 -0300 Subject: [PATCH 15/24] Handle the final TODO --- .../noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs index 180f91acf0c..9fc508618df 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs @@ -189,10 +189,15 @@ impl Context<'_> { let shifted_complement = self.insert_binary(one_complement, BinaryOp::Div, pow); // Convert back to 2-complement representation if operand is negative let lhs_sign_as_int = self.insert_cast(lhs_sign, lhs_typ); - // TODO: should this be unchecked? + + // The requirements for this to underflow are all of these: + // - lhs < 0 + // - ones_complement(lhs) / (2^rhs) == 0 + // As the upper bit is set for the ones complement of negative numbers we'd need 2^rhs + // to be larger than the lhs bitsize for this to overflow. let shifted = self.insert_binary( shifted_complement, - BinaryOp::Sub { unchecked: false }, + BinaryOp::Sub { unchecked: true }, lhs_sign_as_int, ); self.insert_truncate(shifted, bit_size, bit_size + 1) From e0ff78f212914c56f28db51dbb8f9df3fcee6d92 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 12:39:15 -0300 Subject: [PATCH 16/24] Don't emit unchecked operations for Fields, or when multiplying bools --- .../src/ssa/function_builder/mod.rs | 8 --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 41 ++++++++++++- compiler/noirc_evaluator/src/ssa/ir/types.rs | 10 ++++ .../src/ssa/opt/flatten_cfg.rs | 60 +++++++++---------- .../noirc_evaluator/src/ssa/parser/tests.rs | 2 +- 5 files changed, 81 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 d08d5339237..3d7e0b06d5b 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -251,14 +251,6 @@ impl FunctionBuilder { operator: BinaryOp, rhs: ValueId, ) -> ValueId { - let lhs_type = self.type_of_value(lhs); - let rhs_type = self.type_of_value(rhs); - if operator != BinaryOp::Shl && operator != BinaryOp::Shr { - assert_eq!( - lhs_type, rhs_type, - "ICE - Binary instruction operands must have the same type" - ); - } let instruction = Instruction::Binary(Binary { lhs, rhs, operator }); self.insert_instruction(instruction, None).first() } diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 8425e4d5e56..ae8854a95f0 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -7,7 +7,8 @@ use super::{ call_stack::{CallStack, CallStackHelper, CallStackId}, function::{FunctionId, RuntimeType}, instruction::{ - Instruction, InstructionId, InstructionResultType, Intrinsic, TerminatorInstruction, + Binary, BinaryOp, Instruction, InstructionId, InstructionResultType, Intrinsic, + TerminatorInstruction, }, map::DenseMap, types::{NumericType, Type}, @@ -174,6 +175,44 @@ impl DataFlowGraph { instruction_data: Instruction, ctrl_typevars: Option>, ) -> InstructionId { + let instruction_data = if let Instruction::Binary(binary) = instruction_data { + let lhs = binary.lhs; + let rhs = binary.rhs; + let operator = binary.operator; + let lhs_type = self.type_of_value(lhs); + let rhs_type = self.type_of_value(rhs); + if operator != BinaryOp::Shl && operator != BinaryOp::Shr { + assert_eq!( + lhs_type, rhs_type, + "ICE - Binary instruction operands must have the same type" + ); + } + + let operator = if lhs_type.is_field() { + // Unchecked operations between fields or bools don't make sense, so we convert those to non-unchecked + // to reduce noise and confusion in the generated SSA. + match operator { + BinaryOp::Add { unchecked: true } => BinaryOp::Add { unchecked: false }, + BinaryOp::Sub { unchecked: true } => BinaryOp::Sub { unchecked: false }, + BinaryOp::Mul { unchecked: true } => BinaryOp::Mul { unchecked: false }, + _ => operator, + } + } else if lhs_type.is_bool() { + // Unchecked mul between bools doesn't make sense, so we convert that to non-unchecked + if let BinaryOp::Mul { unchecked: true } = operator { + BinaryOp::Mul { unchecked: false } + } else { + operator + } + } else { + operator + }; + + Instruction::Binary(Binary { operator, ..binary }) + } else { + instruction_data + }; + let id = self.instructions.insert(instruction_data); self.make_instruction_results(id, ctrl_typevars); id diff --git a/compiler/noirc_evaluator/src/ssa/ir/types.rs b/compiler/noirc_evaluator/src/ssa/ir/types.rs index 0dd7fd92ee5..2247671a79c 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/types.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/types.rs @@ -119,6 +119,16 @@ impl Type { matches!(self, Type::Numeric(NumericType::Unsigned { .. })) } + /// Returns whether the `Type` represents the Field type. + pub(crate) fn is_field(&self) -> bool { + matches!(self, Type::Numeric(NumericType::NativeField)) + } + + /// Returns whether the `Type` is `u1` + pub(crate) fn is_bool(&self) -> bool { + matches!(self, Type::Numeric(NumericType::Unsigned { bit_size: 1 })) + } + /// Create a new signed integer type with the given amount of bits. pub(crate) fn signed(bit_size: u32) -> Type { Type::Numeric(NumericType::Signed { bit_size }) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 795e06bceb9..76f8495c009 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -850,9 +850,9 @@ mod test { enable_side_effects u1 1 v3 = cast v0 as Field v4 = cast v1 as Field - v6 = unchecked_mul v3, Field 3 - v8 = unchecked_mul v4, Field 4 - v9 = unchecked_add v6, v8 + v6 = mul v3, Field 3 + v8 = mul v4, Field 4 + v9 = add v6, v8 return v9 } "; @@ -881,7 +881,7 @@ mod test { acir(inline) fn main f0 { b0(v0: u1, v1: u1): enable_side_effects v0 - v2 = unchecked_mul v1, v0 + v2 = mul v1, v0 constrain v2 == v0 v3 = not v0 enable_side_effects u1 1 @@ -916,9 +916,9 @@ mod test { v3 = not v0 v4 = cast v0 as Field v5 = cast v3 as Field - v7 = unchecked_mul v4, Field 5 - v8 = unchecked_mul v5, v2 - v9 = unchecked_add v7, v8 + v7 = mul v4, Field 5 + v8 = mul v5, v2 + v9 = add v7, v8 store v9 at v1 enable_side_effects u1 1 return @@ -954,17 +954,17 @@ mod test { v3 = not v0 v4 = cast v0 as Field v5 = cast v3 as Field - v7 = unchecked_mul v4, Field 5 - v8 = unchecked_mul v5, v2 - v9 = unchecked_add v7, v8 + v7 = mul v4, Field 5 + v8 = mul v5, v2 + v9 = add v7, v8 store v9 at v1 enable_side_effects v3 v10 = load v1 -> Field v11 = cast v3 as Field v12 = cast v0 as Field - v14 = unchecked_mul v11, Field 6 - v15 = unchecked_mul v12, v10 - v16 = unchecked_add v14, v15 + v14 = mul v11, Field 6 + v15 = mul v12, v10 + v16 = add v14, v15 store v16 at v1 enable_side_effects u1 1 return @@ -1066,32 +1066,32 @@ mod test { v3 = not v0 v4 = cast v0 as Field v5 = cast v3 as Field - v7 = unchecked_mul v4, Field 2 - v8 = unchecked_add v7, v5 - v9 = unchecked_mul v0, v1 + v7 = mul v4, Field 2 + v8 = add v7, v5 + v9 = mul v0, v1 enable_side_effects v9 v10 = not v9 v11 = cast v9 as Field v12 = cast v10 as Field - v14 = unchecked_mul v11, Field 5 - v15 = unchecked_mul v12, v8 - v16 = unchecked_add v14, v15 + v14 = mul v11, Field 5 + v15 = mul v12, v8 + v16 = add v14, v15 v17 = not v1 - v18 = unchecked_mul v0, v17 + v18 = mul v0, v17 enable_side_effects v18 v19 = not v18 v20 = cast v18 as Field v21 = cast v19 as Field - v23 = unchecked_mul v20, Field 6 - v24 = unchecked_mul v21, v16 - v25 = unchecked_add v23, v24 + v23 = mul v20, Field 6 + v24 = mul v21, v16 + v25 = add v23, v24 enable_side_effects v0 enable_side_effects v3 v26 = cast v3 as Field v27 = cast v0 as Field - v29 = unchecked_mul v26, Field 3 - v30 = unchecked_mul v27, v25 - v31 = unchecked_add v29, v30 + v29 = mul v26, Field 3 + v30 = mul v27, v25 + v31 = add v29, v30 enable_side_effects u1 1 return v31 }"; @@ -1446,9 +1446,9 @@ mod test { v2 = not v0 v3 = cast v0 as Field v4 = cast v2 as Field - v6 = unchecked_mul v3, Field 2 - v7 = unchecked_mul v4, v3 - v8 = unchecked_add v6, v7 + v6 = mul v3, Field 2 + v7 = mul v4, v3 + v8 = add v6, v7 enable_side_effects u1 1 return v8 } @@ -1491,7 +1491,7 @@ mod test { b0(v0: u1, v1: u1): enable_side_effects v0 v2 = cast v0 as Field - v4 = unchecked_mul v2, Field 2 + v4 = mul v2, Field 2 v5 = make_array [v4] : [Field; 1] enable_side_effects u1 1 return v5 diff --git a/compiler/noirc_evaluator/src/ssa/parser/tests.rs b/compiler/noirc_evaluator/src/ssa/parser/tests.rs index ee96d5d82c8..8c24b2ec458 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/tests.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/tests.rs @@ -346,7 +346,7 @@ fn test_binary() { let src = format!( " acir(inline) fn main f0 {{ - b0(v0: Field, v1: Field): + b0(v0: u32, v1: u32): v2 = {op} v0, v1 return }} From bacccc2d427f098e7d7352acc2138cc650d5ce4a Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 12:47:41 -0300 Subject: [PATCH 17/24] Add missing cast in remove_bit_shifts --- compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs index 9fc508618df..29bab66216c 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs @@ -169,6 +169,7 @@ impl Context<'_> { let pow = self.pow(base, rhs); if lhs_typ.is_unsigned() { // unsigned right bit shift is just a normal division + let pow = self.insert_cast(pow, lhs_typ); self.insert_binary(lhs, BinaryOp::Div, pow) } else { // Get the sign of the operand; positive signed operand will just do a division as well From 83f1fd871184c65fd0aab024b533f9b2cff60fcd Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 12:55:21 -0300 Subject: [PATCH 18/24] Extract `self.check_binary_instruction` --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 74 ++++++++++++---------- 1 file changed, 39 insertions(+), 35 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index ae8854a95f0..17ac7b1d38f 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -175,47 +175,51 @@ impl DataFlowGraph { instruction_data: Instruction, ctrl_typevars: Option>, ) -> InstructionId { - let instruction_data = if let Instruction::Binary(binary) = instruction_data { - let lhs = binary.lhs; - let rhs = binary.rhs; - let operator = binary.operator; - let lhs_type = self.type_of_value(lhs); - let rhs_type = self.type_of_value(rhs); - if operator != BinaryOp::Shl && operator != BinaryOp::Shr { - assert_eq!( - lhs_type, rhs_type, - "ICE - Binary instruction operands must have the same type" - ); - } + let instruction_data = self.check_binary_instruction(instruction_data); - let operator = if lhs_type.is_field() { - // Unchecked operations between fields or bools don't make sense, so we convert those to non-unchecked - // to reduce noise and confusion in the generated SSA. - match operator { - BinaryOp::Add { unchecked: true } => BinaryOp::Add { unchecked: false }, - BinaryOp::Sub { unchecked: true } => BinaryOp::Sub { unchecked: false }, - BinaryOp::Mul { unchecked: true } => BinaryOp::Mul { unchecked: false }, - _ => operator, - } - } else if lhs_type.is_bool() { - // Unchecked mul between bools doesn't make sense, so we convert that to non-unchecked - if let BinaryOp::Mul { unchecked: true } = operator { - BinaryOp::Mul { unchecked: false } - } else { - operator - } + let id = self.instructions.insert(instruction_data); + self.make_instruction_results(id, ctrl_typevars); + id + } + + fn check_binary_instruction(&self, instruction: Instruction) -> Instruction { + let Instruction::Binary(binary) = instruction else { + return instruction; + }; + + let lhs = binary.lhs; + let rhs = binary.rhs; + let operator = binary.operator; + let lhs_type = self.type_of_value(lhs); + let rhs_type = self.type_of_value(rhs); + if operator != BinaryOp::Shl && operator != BinaryOp::Shr { + assert_eq!( + lhs_type, rhs_type, + "ICE - Binary instruction operands must have the same type" + ); + } + + let operator = if lhs_type.is_field() { + // Unchecked operations between fields or bools don't make sense, so we convert those to non-unchecked + // to reduce noise and confusion in the generated SSA. + match operator { + BinaryOp::Add { unchecked: true } => BinaryOp::Add { unchecked: false }, + BinaryOp::Sub { unchecked: true } => BinaryOp::Sub { unchecked: false }, + BinaryOp::Mul { unchecked: true } => BinaryOp::Mul { unchecked: false }, + _ => operator, + } + } else if lhs_type.is_bool() { + // Unchecked mul between bools doesn't make sense, so we convert that to non-unchecked + if let BinaryOp::Mul { unchecked: true } = operator { + BinaryOp::Mul { unchecked: false } } else { operator - }; - - Instruction::Binary(Binary { operator, ..binary }) + } } else { - instruction_data + operator }; - let id = self.instructions.insert(instruction_data); - self.make_instruction_results(id, ctrl_typevars); - id + Instruction::Binary(Binary { operator, ..binary }) } /// Check if the function runtime would simply ignore this instruction. From 820c16c6a9f920a73cd45b2f8b24ba9796ba4f05 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 13:08:03 -0300 Subject: [PATCH 19/24] Check binary instruction before simplification --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 17ac7b1d38f..1cd3bfe9830 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -7,7 +7,7 @@ use super::{ call_stack::{CallStack, CallStackHelper, CallStackId}, function::{FunctionId, RuntimeType}, instruction::{ - Binary, BinaryOp, Instruction, InstructionId, InstructionResultType, Intrinsic, + self, Binary, BinaryOp, Instruction, InstructionId, InstructionResultType, Intrinsic, TerminatorInstruction, }, map::DenseMap, @@ -175,8 +175,6 @@ impl DataFlowGraph { instruction_data: Instruction, ctrl_typevars: Option>, ) -> InstructionId { - let instruction_data = self.check_binary_instruction(instruction_data); - let id = self.instructions.insert(instruction_data); self.make_instruction_results(id, ctrl_typevars); id @@ -251,6 +249,8 @@ impl DataFlowGraph { return InsertInstructionResult::InstructionRemoved; } + let instruction_data = self.check_binary_instruction(instruction_data); + let id = self.insert_instruction_without_simplification( instruction_data, block, @@ -273,6 +273,9 @@ impl DataFlowGraph { if !self.is_handled_by_runtime(&instruction) { return InsertInstructionResult::InstructionRemoved; } + + let instruction = self.check_binary_instruction(instruction); + match instruction.simplify(self, block, ctrl_typevars.clone(), call_stack) { SimplifyResult::SimplifiedTo(simplification) => { InsertInstructionResult::SimplifiedTo(simplification) From 4683b0dd7d4839f115f56a9a8bab4f108e22aba6 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 13:08:39 -0300 Subject: [PATCH 20/24] clippy --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 1cd3bfe9830..6afa9d7dc79 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -7,7 +7,7 @@ use super::{ call_stack::{CallStack, CallStackHelper, CallStackId}, function::{FunctionId, RuntimeType}, instruction::{ - self, Binary, BinaryOp, Instruction, InstructionId, InstructionResultType, Intrinsic, + Binary, BinaryOp, Instruction, InstructionId, InstructionResultType, Intrinsic, TerminatorInstruction, }, map::DenseMap, From cf1a9e73d45ea95b2ef775ecbbda94c6e2b920da Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 13:11:57 -0300 Subject: [PATCH 21/24] Do cast in both cases --- compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs index 29bab66216c..e36be71aeea 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs @@ -167,9 +167,9 @@ impl Context<'_> { let lhs_typ = self.function.dfg.type_of_value(lhs).unwrap_numeric(); let base = self.field_constant(FieldElement::from(2_u128)); let pow = self.pow(base, rhs); + let pow = self.insert_cast(pow, lhs_typ); if lhs_typ.is_unsigned() { // unsigned right bit shift is just a normal division - let pow = self.insert_cast(pow, lhs_typ); self.insert_binary(lhs, BinaryOp::Div, pow) } else { // Get the sign of the operand; positive signed operand will just do a division as well From ef30d7ccea25d13629ae1a3d5216fcfc45975412 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 13:24:05 -0300 Subject: [PATCH 22/24] Only check and simplify binary instructions in `Binary::simplify` --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 47 +-------- .../src/ssa/ir/instruction/binary.rs | 99 ++++++++++++------- compiler/noirc_evaluator/src/ssa/ir/types.rs | 10 -- 3 files changed, 67 insertions(+), 89 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 6afa9d7dc79..8a4b3fa1ce6 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -7,8 +7,7 @@ use super::{ call_stack::{CallStack, CallStackHelper, CallStackId}, function::{FunctionId, RuntimeType}, instruction::{ - Binary, BinaryOp, Instruction, InstructionId, InstructionResultType, Intrinsic, - TerminatorInstruction, + Instruction, InstructionId, InstructionResultType, Intrinsic, TerminatorInstruction, }, map::DenseMap, types::{NumericType, Type}, @@ -180,46 +179,6 @@ impl DataFlowGraph { id } - fn check_binary_instruction(&self, instruction: Instruction) -> Instruction { - let Instruction::Binary(binary) = instruction else { - return instruction; - }; - - let lhs = binary.lhs; - let rhs = binary.rhs; - let operator = binary.operator; - let lhs_type = self.type_of_value(lhs); - let rhs_type = self.type_of_value(rhs); - if operator != BinaryOp::Shl && operator != BinaryOp::Shr { - assert_eq!( - lhs_type, rhs_type, - "ICE - Binary instruction operands must have the same type" - ); - } - - let operator = if lhs_type.is_field() { - // Unchecked operations between fields or bools don't make sense, so we convert those to non-unchecked - // to reduce noise and confusion in the generated SSA. - match operator { - BinaryOp::Add { unchecked: true } => BinaryOp::Add { unchecked: false }, - BinaryOp::Sub { unchecked: true } => BinaryOp::Sub { unchecked: false }, - BinaryOp::Mul { unchecked: true } => BinaryOp::Mul { unchecked: false }, - _ => operator, - } - } else if lhs_type.is_bool() { - // Unchecked mul between bools doesn't make sense, so we convert that to non-unchecked - if let BinaryOp::Mul { unchecked: true } = operator { - BinaryOp::Mul { unchecked: false } - } else { - operator - } - } else { - operator - }; - - Instruction::Binary(Binary { operator, ..binary }) - } - /// Check if the function runtime would simply ignore this instruction. pub(crate) fn is_handled_by_runtime(&self, instruction: &Instruction) -> bool { !(self.runtime().is_acir() && instruction.is_brillig_only()) @@ -249,8 +208,6 @@ impl DataFlowGraph { return InsertInstructionResult::InstructionRemoved; } - let instruction_data = self.check_binary_instruction(instruction_data); - let id = self.insert_instruction_without_simplification( instruction_data, block, @@ -274,8 +231,6 @@ impl DataFlowGraph { return InsertInstructionResult::InstructionRemoved; } - let instruction = self.check_binary_instruction(instruction); - match instruction.simplify(self, block, ctrl_typevars.clone(), call_stack) { SimplifyResult::SimplifiedTo(simplification) => { InsertInstructionResult::SimplifiedTo(simplification) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs index 79817edb3e5..df1e8f537da 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs @@ -89,25 +89,58 @@ impl Binary { /// Try to simplify this binary instruction, returning the new value if possible. pub(super) fn simplify(&self, dfg: &mut DataFlowGraph) -> SimplifyResult { - let lhs = dfg.get_numeric_constant(self.lhs); - let rhs = dfg.get_numeric_constant(self.rhs); - let operand_type = dfg.type_of_value(self.lhs).unwrap_numeric(); + let lhs_value = dfg.get_numeric_constant(self.lhs); + let rhs_value = dfg.get_numeric_constant(self.rhs); + + let lhs_type = dfg.type_of_value(self.lhs).unwrap_numeric(); + let rhs_type = dfg.type_of_value(self.rhs).unwrap_numeric(); + + let operator = self.operator; + if operator != BinaryOp::Shl && operator != BinaryOp::Shr { + assert_eq!( + lhs_type, rhs_type, + "ICE - Binary instruction operands must have the same type" + ); + } + + let operator = if lhs_type == NumericType::NativeField { + // Unchecked operations between fields or bools don't make sense, so we convert those to non-unchecked + // to reduce noise and confusion in the generated SSA. + match operator { + BinaryOp::Add { unchecked: true } => BinaryOp::Add { unchecked: false }, + BinaryOp::Sub { unchecked: true } => BinaryOp::Sub { unchecked: false }, + BinaryOp::Mul { unchecked: true } => BinaryOp::Mul { unchecked: false }, + _ => operator, + } + } else if lhs_type == NumericType::bool() { + // Unchecked mul between bools doesn't make sense, so we convert that to non-unchecked + if let BinaryOp::Mul { unchecked: true } = operator { + BinaryOp::Mul { unchecked: false } + } else { + operator + } + } else { + operator + }; + + // We never return `SimplifyResult::None` here because `operator` might have changed. + let simplified = Instruction::Binary(Binary { lhs: self.lhs, rhs: self.rhs, operator }); - if let (Some(lhs), Some(rhs)) = (lhs, rhs) { - return match eval_constant_binary_op(lhs, rhs, self.operator, operand_type) { + if let (Some(lhs), Some(rhs)) = (lhs_value, rhs_value) { + return match eval_constant_binary_op(lhs, rhs, operator, lhs_type) { Some((result, result_type)) => { let value = dfg.make_constant(result, result_type); SimplifyResult::SimplifiedTo(value) } - None => SimplifyResult::None, + None => SimplifyResult::SimplifiedToInstruction(simplified), }; } - let lhs_is_zero = lhs.map_or(false, |lhs| lhs.is_zero()); - let rhs_is_zero = rhs.map_or(false, |rhs| rhs.is_zero()); + let lhs_is_zero = lhs_value.map_or(false, |lhs| lhs.is_zero()); + let rhs_is_zero = rhs_value.map_or(false, |rhs| rhs.is_zero()); - let lhs_is_one = lhs.map_or(false, |lhs| lhs.is_one()); - let rhs_is_one = rhs.map_or(false, |rhs| rhs.is_one()); + let lhs_is_one = lhs_value.map_or(false, |lhs| lhs.is_one()); + let rhs_is_one = rhs_value.map_or(false, |rhs| rhs.is_one()); match self.operator { BinaryOp::Add { .. } => { @@ -131,7 +164,7 @@ impl Binary { return SimplifyResult::SimplifiedTo(self.lhs); } if lhs_is_zero || rhs_is_zero { - let zero = dfg.make_constant(FieldElement::zero(), operand_type); + let zero = dfg.make_constant(FieldElement::zero(), lhs_type); return SimplifyResult::SimplifiedTo(zero); } if dfg.get_value_max_num_bits(self.lhs) == 1 { @@ -176,13 +209,13 @@ impl Binary { } BinaryOp::Mod => { if rhs_is_one { - let zero = dfg.make_constant(FieldElement::zero(), operand_type); + let zero = dfg.make_constant(FieldElement::zero(), lhs_type); return SimplifyResult::SimplifiedTo(zero); } - if operand_type.is_unsigned() { + if lhs_type.is_unsigned() { // lhs % 2**bit_size is equivalent to truncating `lhs` to `bit_size` bits. // We then convert to a truncation for consistency, allowing more optimizations. - if let Some(modulus) = rhs { + if let Some(modulus) = rhs_value { let modulus = modulus.to_u128(); if modulus.is_power_of_two() { let bit_size = modulus.ilog2(); @@ -190,7 +223,7 @@ impl Binary { Instruction::Truncate { value: self.lhs, bit_size, - max_bit_size: operand_type.bit_size(), + max_bit_size: lhs_type.bit_size(), }, ); } @@ -203,7 +236,7 @@ impl Binary { return SimplifyResult::SimplifiedTo(one); } - if operand_type == NumericType::bool() { + if lhs_type == NumericType::bool() { // Simplify forms of `(boolean == true)` into `boolean` if lhs_is_one { return SimplifyResult::SimplifiedTo(self.rhs); @@ -225,13 +258,13 @@ impl Binary { let zero = dfg.make_constant(FieldElement::zero(), NumericType::bool()); return SimplifyResult::SimplifiedTo(zero); } - if operand_type.is_unsigned() { + if lhs_type.is_unsigned() { if rhs_is_zero { // Unsigned values cannot be less than zero. let zero = dfg.make_constant(FieldElement::zero(), NumericType::bool()); return SimplifyResult::SimplifiedTo(zero); } else if rhs_is_one { - let zero = dfg.make_constant(FieldElement::zero(), operand_type); + let zero = dfg.make_constant(FieldElement::zero(), lhs_type); return SimplifyResult::SimplifiedToInstruction(Instruction::binary( BinaryOp::Eq, self.lhs, @@ -242,37 +275,37 @@ impl Binary { } BinaryOp::And => { if lhs_is_zero || rhs_is_zero { - let zero = dfg.make_constant(FieldElement::zero(), operand_type); + let zero = dfg.make_constant(FieldElement::zero(), lhs_type); return SimplifyResult::SimplifiedTo(zero); } if dfg.resolve(self.lhs) == dfg.resolve(self.rhs) { return SimplifyResult::SimplifiedTo(self.lhs); } - if operand_type == NumericType::bool() { + if lhs_type == NumericType::bool() { // Boolean AND is equivalent to multiplication, which is a cheaper operation. // (mul unchecked because these are bools so it doesn't matter really) let instruction = Instruction::binary(BinaryOp::Mul { unchecked: true }, self.lhs, self.rhs); return SimplifyResult::SimplifiedToInstruction(instruction); } - if operand_type.is_unsigned() { + if lhs_type.is_unsigned() { // It's common in other programming languages to truncate values to a certain bit size using // a bitwise AND with a bit mask. However this operation is quite inefficient inside a snark. // // We then replace this bitwise operation with an equivalent truncation instruction. - match (lhs, rhs) { + match (lhs_value, rhs_value) { (Some(bitmask), None) | (None, Some(bitmask)) => { // This substitution requires the bitmask to retain all of the lower bits. // The bitmask must then be one less than a power of 2. let bitmask_plus_one = bitmask.to_u128() + 1; if bitmask_plus_one.is_power_of_two() { - let value = if lhs.is_some() { self.rhs } else { self.lhs }; + let value = if lhs_value.is_some() { self.rhs } else { self.lhs }; let num_bits = bitmask_plus_one.ilog2(); return SimplifyResult::SimplifiedToInstruction( Instruction::Truncate { value, bit_size: num_bits, - max_bit_size: operand_type.bit_size(), + max_bit_size: lhs_type.bit_size(), }, ); } @@ -289,8 +322,8 @@ impl Binary { if rhs_is_zero { return SimplifyResult::SimplifiedTo(self.lhs); } - if operand_type == NumericType::bool() && (lhs_is_one || rhs_is_one) { - let one = dfg.make_constant(FieldElement::one(), operand_type); + if lhs_type == NumericType::bool() && (lhs_is_one || rhs_is_one) { + let one = dfg.make_constant(FieldElement::one(), lhs_type); return SimplifyResult::SimplifiedTo(one); } if dfg.resolve(self.lhs) == dfg.resolve(self.rhs) { @@ -305,24 +338,24 @@ impl Binary { return SimplifyResult::SimplifiedTo(self.lhs); } if dfg.resolve(self.lhs) == dfg.resolve(self.rhs) { - let zero = dfg.make_constant(FieldElement::zero(), operand_type); + let zero = dfg.make_constant(FieldElement::zero(), lhs_type); return SimplifyResult::SimplifiedTo(zero); } } - BinaryOp::Shl => return SimplifyResult::None, + BinaryOp::Shl => return SimplifyResult::SimplifiedToInstruction(simplified), BinaryOp::Shr => { // Bit shifts by constants can be treated as divisions. - if let Some(rhs_const) = rhs { - if rhs_const >= FieldElement::from(operand_type.bit_size() as u128) { + if let Some(rhs_const) = rhs_value { + if rhs_const >= FieldElement::from(lhs_type.bit_size() as u128) { // Shifting by the full width of the operand type, any `lhs` goes to zero. - let zero = dfg.make_constant(FieldElement::zero(), operand_type); + let zero = dfg.make_constant(FieldElement::zero(), lhs_type); return SimplifyResult::SimplifiedTo(zero); } - return SimplifyResult::None; + return SimplifyResult::SimplifiedToInstruction(simplified); } } }; - SimplifyResult::None + SimplifyResult::SimplifiedToInstruction(simplified) } /// Check if unsigned overflow is possible, and if so return some message to be used if it fails. diff --git a/compiler/noirc_evaluator/src/ssa/ir/types.rs b/compiler/noirc_evaluator/src/ssa/ir/types.rs index 2247671a79c..0dd7fd92ee5 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/types.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/types.rs @@ -119,16 +119,6 @@ impl Type { matches!(self, Type::Numeric(NumericType::Unsigned { .. })) } - /// Returns whether the `Type` represents the Field type. - pub(crate) fn is_field(&self) -> bool { - matches!(self, Type::Numeric(NumericType::NativeField)) - } - - /// Returns whether the `Type` is `u1` - pub(crate) fn is_bool(&self) -> bool { - matches!(self, Type::Numeric(NumericType::Unsigned { bit_size: 1 })) - } - /// Create a new signed integer type with the given amount of bits. pub(crate) fn signed(bit_size: u32) -> Type { Type::Numeric(NumericType::Signed { bit_size }) From 63cfd125975c96a51780ded9d7f0b3c97cb368db Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 14:00:50 -0300 Subject: [PATCH 23/24] Fix comptime to_le_bits returning `[u8; N]` instead of `[u1; N]` --- .../src/hir/comptime/interpreter/builtin.rs | 31 ++++++++++++++----- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index 730bcd1be6c..3d3d0721c65 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -822,10 +822,10 @@ fn to_le_radix( let value = get_field(value)?; let radix = get_u32(radix)?; - let limb_count = if let Type::Array(length, _) = return_type { + let (limb_count, element_type) = if let Type::Array(length, element_type) = return_type { if let Type::Constant(limb_count, kind) = *length { if kind.unifies(&Kind::u32()) { - limb_count + (limb_count, element_type) } else { return Err(InterpreterError::TypeAnnotationsNeededForMethodCall { location }); } @@ -836,14 +836,29 @@ fn to_le_radix( return Err(InterpreterError::TypeAnnotationsNeededForMethodCall { location }); }; + let return_type_is_bits = + *element_type == Type::Integer(Signedness::Unsigned, IntegerBitSize::One); + // Decompose the integer into its radix digits in little endian form. let decomposed_integer = compute_to_radix_le(value, radix); - let decomposed_integer = - vecmap(0..limb_count.to_u128() as usize, |i| match decomposed_integer.get(i) { - Some(digit) => Value::U8(*digit), - None => Value::U8(0), - }); - let result_type = byte_array_type(decomposed_integer.len()); + let decomposed_integer = vecmap(0..limb_count.to_u128() as usize, |i| { + let digit = match decomposed_integer.get(i) { + Some(digit) => *digit, + None => 0, + }; + // The only built-ins that use these either return `[u1; N]` or `[u8; N]` + if return_type_is_bits { + Value::U1(digit != 0) + } else { + Value::U8(digit) + } + }); + + let result_type = Type::Array( + Box::new(Type::Constant(decomposed_integer.len().into(), Kind::u32())), + element_type, + ); + Ok(Value::Array(decomposed_integer.into(), result_type)) } From 9b42fd8c17143e939255c671542db28cbac4e776 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 14:01:38 -0300 Subject: [PATCH 24/24] Allow casting u1 in comptime --- compiler/noirc_frontend/src/hir/comptime/interpreter.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index 13a9400bd2e..ec4d33c3ca4 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -1424,6 +1424,7 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { let (mut lhs, lhs_is_negative) = match evaluated_lhs { Value::Field(value) => (value, false), + Value::U1(value) => ((value as u128).into(), false), Value::U8(value) => ((value as u128).into(), false), Value::U16(value) => ((value as u128).into(), false), Value::U32(value) => ((value as u128).into(), false),