From 7f992d95f1e933ec31a787476ddc6eded180defd Mon Sep 17 00:00:00 2001 From: Tom French Date: Sun, 14 Jan 2024 23:41:16 +0000 Subject: [PATCH 1/4] feat: Avoid unnecessary range checks by inspecting instructions for casts --- .../src/ssa/function_builder/mod.rs | 18 ++- .../src/ssa/ssa_gen/context.rs | 107 ++++++++++++++---- 2 files changed, 101 insertions(+), 24 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 2871f149b41..3a57f52a1f2 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -293,8 +293,9 @@ impl FunctionBuilder { if let Some(rhs_constant) = self.current_function.dfg.get_numeric_constant(rhs) { // Happy case is that we know precisely by how many bits the the integer will // increase: lhs_bit_size + rhs - let (rhs_bit_size_pow_2, overflows) = - 2_u128.overflowing_pow(rhs_constant.to_u128() as u32); + let bit_shift_size = rhs_constant.to_u128() as u32; + + let (rhs_bit_size_pow_2, overflows) = 2_u128.overflowing_pow(bit_shift_size); if overflows { assert!(bit_size < 128, "ICE - shift left with big integers are not supported"); if bit_size < 128 { @@ -303,7 +304,18 @@ impl FunctionBuilder { } } let pow = self.numeric_constant(FieldElement::from(rhs_bit_size_pow_2), typ); - (bit_size + (rhs_constant.to_u128() as u32), pow) + + let mut max_lhs_bit = bit_size; + + let dfg = &self.current_function.dfg; + if let Value::Instruction { instruction, .. } = dfg[lhs] { + if let Instruction::Cast(original_lhs, _) = dfg[instruction] { + let original_type = self.type_of_value(original_lhs); + max_lhs_bit = original_type.bit_size(); + } + } + + (max_lhs_bit + bit_shift_size, pow) } else { // we use a predicate to nullify the result in case of overflow let bit_size_var = diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index b34b667c31a..19de11b6aab 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -17,7 +17,7 @@ use crate::ssa::ir::instruction::BinaryOp; use crate::ssa::ir::instruction::Instruction; use crate::ssa::ir::map::AtomicCounter; use crate::ssa::ir::types::{NumericType, Type}; -use crate::ssa::ir::value::ValueId; +use crate::ssa::ir::value::{Value as IrValue, ValueId}; use super::value::{Tree, Value, Values}; use fxhash::FxHashMap as HashMap; @@ -335,29 +335,94 @@ impl<'a> FunctionContext<'a> { } } Type::Numeric(NumericType::Unsigned { bit_size }) => { - let op_name = match operator { - BinaryOpKind::Add => "add", - BinaryOpKind::Subtract => "subtract", - BinaryOpKind::Multiply => "multiply", - BinaryOpKind::ShiftLeft => "left shift", - _ => unreachable!("operator {} should not overflow", operator), + let dfg = &self.builder.current_function.dfg; + + let max_lhs_bits = match dfg[lhs] { + IrValue::Instruction { instruction, .. } => { + if let Instruction::Cast(original_lhs, _) = dfg[instruction] { + self.builder.type_of_value(original_lhs).bit_size() + } else { + self.builder.type_of_value(lhs).bit_size() + } + } + + IrValue::NumericConstant { constant, .. } => constant.num_bits(), + _ => self.builder.type_of_value(lhs).bit_size(), }; - if operator == BinaryOpKind::Multiply && bit_size == 1 { - result - } else if operator == BinaryOpKind::ShiftLeft - || operator == BinaryOpKind::ShiftRight - { - self.check_shift_overflow(result, rhs, bit_size, location, false) - } else { - let message = format!("attempt to {} with overflow", op_name); - self.builder.set_location(location).insert_range_check( - result, - bit_size, - Some(message), - ); - result + let max_rhs_bits = match dfg[rhs] { + IrValue::Instruction { instruction, .. } => { + if let Instruction::Cast(original_rhs, _) = dfg[instruction] { + self.builder.type_of_value(original_rhs).bit_size() + } else { + self.builder.type_of_value(rhs).bit_size() + } + } + + IrValue::NumericConstant { constant, .. } => constant.num_bits(), + _ => self.builder.type_of_value(rhs).bit_size(), + }; + + match operator { + BinaryOpKind::Add => { + 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 result; + } + + let message = "attempt to add with overflow".to_string(); + self.builder.set_location(location).insert_range_check( + result, + bit_size, + Some(message), + ); + } + BinaryOpKind::Subtract => { + if dfg.is_constant(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. + return result; + } + + let message = "attempt to subtract with overflow".to_string(); + self.builder.set_location(location).insert_range_check( + result, + bit_size, + Some(message), + ); + } + BinaryOpKind::Multiply => { + if bit_size == 1 || max_lhs_bits + max_rhs_bits <= bit_size { + // Either performing boolean multiplication (which cannot overflow), + // or `lhs` and `rhs` have both been casted up from smaller types and so cannot overflow. + return result; + } + + let message = "attempt to multiply with overflow".to_string(); + self.builder.set_location(location).insert_range_check( + result, + bit_size, + Some(message), + ); + } + BinaryOpKind::ShiftLeft => { + if let Some(rhs_const) = dfg.get_numeric_constant(rhs) { + let bit_shift_size = rhs_const.to_u128() as u32; + + if max_lhs_bits + bit_shift_size <= bit_size { + // `lhs` has been casted up from a smaller type such that shifting it by a constant + // `rhs` is known not to exceed the maximum bit size. + return result; + } + } + + self.check_shift_overflow(result, rhs, bit_size, location, false); + } + + _ => unreachable!("operator {} should not overflow", operator), } + + result } _ => result, } From 076922cc67656c8722e13505683afcdb508eea88 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Mon, 15 Jan 2024 12:00:01 +0000 Subject: [PATCH 2/4] chore: add helper function to get bit size of value --- .../src/ssa/ssa_gen/context.rs | 47 +++++++++---------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 19de11b6aab..7886def0b3b 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -287,6 +287,26 @@ impl<'a> FunctionContext<'a> { self.builder.insert_binary(positive_predicate, BinaryOp::Add, negative_predicate) } + /// Returns the maximum possible number of bits that `value` can potentially be. + /// + /// Should `value` be a numeric constant then this function will return the exact number of bits required, + /// otherwise it will return the minimum number of bits based on type information. + fn get_value_max_num_bits(&self, value: ValueId) -> u32 { + let dfg = &self.builder.current_function.dfg; + match dfg[value] { + IrValue::Instruction { instruction, .. } => { + if let Instruction::Cast(original_value, _) = dfg[instruction] { + self.builder.type_of_value(original_value).bit_size() + } else { + self.builder.type_of_value(value).bit_size() + } + } + + IrValue::NumericConstant { constant, .. } => constant.num_bits(), + _ => self.builder.type_of_value(value).bit_size(), + } + } + /// Insert constraints ensuring that the operation does not overflow the bit size of the result /// /// If the result is unsigned, we simply range check against the bit size @@ -337,31 +357,8 @@ impl<'a> FunctionContext<'a> { Type::Numeric(NumericType::Unsigned { bit_size }) => { let dfg = &self.builder.current_function.dfg; - let max_lhs_bits = match dfg[lhs] { - IrValue::Instruction { instruction, .. } => { - if let Instruction::Cast(original_lhs, _) = dfg[instruction] { - self.builder.type_of_value(original_lhs).bit_size() - } else { - self.builder.type_of_value(lhs).bit_size() - } - } - - IrValue::NumericConstant { constant, .. } => constant.num_bits(), - _ => self.builder.type_of_value(lhs).bit_size(), - }; - - let max_rhs_bits = match dfg[rhs] { - IrValue::Instruction { instruction, .. } => { - if let Instruction::Cast(original_rhs, _) = dfg[instruction] { - self.builder.type_of_value(original_rhs).bit_size() - } else { - self.builder.type_of_value(rhs).bit_size() - } - } - - IrValue::NumericConstant { constant, .. } => constant.num_bits(), - _ => self.builder.type_of_value(rhs).bit_size(), - }; + let max_lhs_bits = self.get_value_max_num_bits(lhs); + let max_rhs_bits = self.get_value_max_num_bits(rhs); match operator { BinaryOpKind::Add => { From b18aa8b171fb539072e6fa376d8ae0328b8db939 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Mon, 15 Jan 2024 14:04:20 +0000 Subject: [PATCH 3/4] chore: move method to `dfg` --- .../src/ssa/function_builder/mod.rs | 12 ++-------- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 19 +++++++++++++++ .../src/ssa/ssa_gen/context.rs | 24 ++----------------- 3 files changed, 23 insertions(+), 32 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 3a57f52a1f2..35782ea85ae 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -305,17 +305,9 @@ impl FunctionBuilder { } let pow = self.numeric_constant(FieldElement::from(rhs_bit_size_pow_2), typ); - let mut max_lhs_bit = bit_size; + let max_lhs_bits = self.current_function.dfg.get_value_max_num_bits(lhs); - let dfg = &self.current_function.dfg; - if let Value::Instruction { instruction, .. } = dfg[lhs] { - if let Instruction::Cast(original_lhs, _) = dfg[instruction] { - let original_type = self.type_of_value(original_lhs); - max_lhs_bit = original_type.bit_size(); - } - } - - (max_lhs_bit + bit_shift_size, pow) + (max_lhs_bits + bit_shift_size, pow) } else { // we use a predicate to nullify the result in case of overflow let bit_size_var = diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 9942a48a38a..870b5e602f1 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -337,6 +337,25 @@ impl DataFlowGraph { self.values[value].get_type().clone() } + /// Returns the maximum possible number of bits that `value` can potentially be. + /// + /// Should `value` be a numeric constant then this function will return the exact number of bits required, + /// otherwise it will return the minimum number of bits based on type information. + pub(crate) fn get_value_max_num_bits(&self, value: ValueId) -> u32 { + match self[value] { + Value::Instruction { instruction, .. } => { + if let Instruction::Cast(original_value, _) = self[instruction] { + self.type_of_value(original_value).bit_size() + } else { + self.type_of_value(value).bit_size() + } + } + + Value::NumericConstant { constant, .. } => constant.num_bits(), + _ => self.type_of_value(value).bit_size(), + } + } + /// True if the type of this value is Type::Reference. /// Using this method over type_of_value avoids cloning the value's type. pub(crate) fn value_is_reference(&self, value: ValueId) -> bool { diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 7886def0b3b..42d2e86a46c 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -287,26 +287,6 @@ impl<'a> FunctionContext<'a> { self.builder.insert_binary(positive_predicate, BinaryOp::Add, negative_predicate) } - /// Returns the maximum possible number of bits that `value` can potentially be. - /// - /// Should `value` be a numeric constant then this function will return the exact number of bits required, - /// otherwise it will return the minimum number of bits based on type information. - fn get_value_max_num_bits(&self, value: ValueId) -> u32 { - let dfg = &self.builder.current_function.dfg; - match dfg[value] { - IrValue::Instruction { instruction, .. } => { - if let Instruction::Cast(original_value, _) = dfg[instruction] { - self.builder.type_of_value(original_value).bit_size() - } else { - self.builder.type_of_value(value).bit_size() - } - } - - IrValue::NumericConstant { constant, .. } => constant.num_bits(), - _ => self.builder.type_of_value(value).bit_size(), - } - } - /// Insert constraints ensuring that the operation does not overflow the bit size of the result /// /// If the result is unsigned, we simply range check against the bit size @@ -357,8 +337,8 @@ impl<'a> FunctionContext<'a> { Type::Numeric(NumericType::Unsigned { bit_size }) => { let dfg = &self.builder.current_function.dfg; - let max_lhs_bits = self.get_value_max_num_bits(lhs); - let max_rhs_bits = self.get_value_max_num_bits(rhs); + let max_lhs_bits = self.builder.current_function.dfg.get_value_max_num_bits(lhs); + let max_rhs_bits = self.builder.current_function.dfg.get_value_max_num_bits(rhs); match operator { BinaryOpKind::Add => { From 94e5d79ed11691103c6fc1c17c6085d52316d005 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Mon, 15 Jan 2024 15:22:51 +0000 Subject: [PATCH 4/4] chore: remove unused import --- 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 42d2e86a46c..f1a2154d3a8 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -17,7 +17,7 @@ use crate::ssa::ir::instruction::BinaryOp; use crate::ssa::ir::instruction::Instruction; use crate::ssa::ir::map::AtomicCounter; use crate::ssa::ir::types::{NumericType, Type}; -use crate::ssa::ir::value::{Value as IrValue, ValueId}; +use crate::ssa::ir::value::ValueId; use super::value::{Tree, Value, Values}; use fxhash::FxHashMap as HashMap;