fix: Don't skip for loops with negative literal ranges

compiler/noirc_evaluator/src/ssa/ir/dfg.rs

@@ -12,6 +12,7 @@ use super::{
     instruction::{
         Instruction, InstructionId, InstructionResultType, Intrinsic, TerminatorInstruction,
     },
+    integer::IntegerConstant,
     map::DenseMap,
     types::{NumericType, Type},
     value::{Value, ValueId, ValueMapping},
@@ -582,13 +583,22 @@ impl DataFlowGraph {
     }
 
     /// Returns the field element represented by this value if it is a numeric constant.
-    /// Returns None if the given value is not a numeric constant.
+    /// Returns `None` if the given value is not a numeric constant.
+    ///
+    /// Use `get_integer_constant` if the underlying values need to be compared as signed integers.
     pub(crate) fn get_numeric_constant(&self, value: ValueId) -> Option<FieldElement> {
         self.get_numeric_constant_with_type(value).map(|(value, _typ)| value)
     }
 
+    /// Similar to `get_numeric_constant` but returns the value as a signed or unsigned integer.
+    /// Returns `None` if the given value is not an integer constant.
+    pub(crate) fn get_integer_constant(&self, value: ValueId) -> Option<IntegerConstant> {
+        self.get_numeric_constant_with_type(value)
+            .and_then(|(f, t)| IntegerConstant::from_numeric_constant(f, t))
+    }
+
     /// Returns the field element and type represented by this value if it is a numeric constant.
-    /// Returns None if the given value is not a numeric constant.
+    /// Returns `None` if the given value is not a numeric constant.
     pub(crate) fn get_numeric_constant_with_type(
         &self,
         value: ValueId,

compiler/noirc_evaluator/src/ssa/ir/integer.rs

@@ -0,0 +1,122 @@
+use std::cmp::Ordering;
+
+use acvm::{AcirField, FieldElement};
+use num_traits::Zero;
+
+use super::{instruction::binary, types::NumericType};
+
+/// A `Signed` or `Unsigned` value of a `Value::NumericConstant`, converted to 128 bits.
+///
+/// This type can be used in loops and other instances where values have to be compared,
+/// with correct handling of negative values.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub(crate) enum IntegerConstant {
+    Signed { value: i128, bit_size: u32 },
+    Unsigned { value: u128, bit_size: u32 },
+}
+
+impl IntegerConstant {
+    pub(crate) fn from_numeric_constant(field: FieldElement, typ: NumericType) -> Option<Self> {
+        match typ {
+            NumericType::Signed { bit_size } => {
+                binary::try_convert_field_element_to_signed_integer(field, bit_size)
+                    .map(|value| Self::Signed { value, bit_size })
+            }
+            NumericType::Unsigned { bit_size } => {
+                Some(Self::Unsigned { value: field.to_u128(), bit_size })
+            }
+            NumericType::NativeField => None,
+        }
+    }
+
+    /// Convert back into a field.
+    pub(crate) fn into_numeric_constant(self) -> (FieldElement, NumericType) {
+        match self {
+            Self::Signed { value, bit_size } => (
+                binary::convert_signed_integer_to_field_element(value, bit_size),
+                NumericType::signed(bit_size),
+            ),
+            Self::Unsigned { value, bit_size } => {
+                (FieldElement::from(value), NumericType::unsigned(bit_size))
+            }
+        }
+    }
+
+    /// Reduce two constants into a result by applying functions on them if their signedness matches.
+    pub(crate) fn reduce<T>(
+        self,
+        other: Self,
+        s: impl Fn(i128, i128) -> T,
+        u: impl Fn(u128, u128) -> T,
+    ) -> Option<T> {
+        match (self, other) {
+            (Self::Signed { value: a, .. }, Self::Signed { value: b, .. }) => Some(s(a, b)),
+            (Self::Unsigned { value: a, .. }, Self::Unsigned { value: b, .. }) => Some(u(a, b)),
+            _ => None,
+        }
+    }
+
+    /// Apply functions on signed/unsigned values.
+    pub(crate) fn apply<T>(&self, s: impl Fn(i128) -> T, u: impl Fn(u128) -> T) -> T {
+        match self {
+            Self::Signed { value, .. } => s(*value),
+            Self::Unsigned { value, .. } => u(*value),
+        }
+    }
+
+    /// Increment the value by 1, saturating at the maximum value.
+    pub(crate) fn inc(self) -> Self {
+        match self {
+            Self::Signed { value, bit_size } => {
+                Self::Signed { value: value.saturating_add(1), bit_size }
+            }
+            Self::Unsigned { value, bit_size } => {
+                Self::Unsigned { value: value.saturating_add(1), bit_size }
+            }
+        }
+    }
+
+    /// Decrement the value by 1, saturating at the minimum value.
+    pub(crate) fn dec(self) -> Self {
+        match self {
+            Self::Signed { value, bit_size } => {
+                Self::Signed { value: value.saturating_sub(1), bit_size }
+            }
+            Self::Unsigned { value, bit_size } => {
+                Self::Unsigned { value: value.saturating_sub(1), bit_size }
+            }
+        }
+    }
+
+    pub(crate) fn is_zero(&self) -> bool {
+        match self {
+            Self::Signed { value, .. } => value.is_zero(),
+            Self::Unsigned { value, .. } => value.is_zero(),
+        }
+    }
+}
+
+impl PartialOrd for IntegerConstant {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        match (self, other) {
+            (Self::Signed { value: a, .. }, Self::Signed { value: b, .. }) => a.partial_cmp(b),
+            (Self::Signed { value: a, .. }, Self::Unsigned { value: b, .. }) => {
+                if a.is_negative() || *b > i128::MAX as u128 {
+                    Some(Ordering::Less)
+                } else {
+                    let a: u128 = (*a).try_into().ok()?;
+                    a.partial_cmp(b)
+                }
+            }
+            (Self::Unsigned { value: a, .. }, Self::Signed { value: b, .. }) => {
+                if b.is_negative() || *a > i128::MAX as u128 {
+                    Some(Ordering::Less)
+                } else {
+                    let b: u128 = (*b).try_into().ok()?;
+                    a.partial_cmp(&b)
+                }
+            }
+            (Self::Unsigned { value: a, .. }, Self::Unsigned { value: b, .. }) => a.partial_cmp(b),
+        }
+    }
+}

compiler/noirc_evaluator/src/ssa/ir/mod.rs

@@ -6,6 +6,7 @@ pub(crate) mod dom;
 pub mod function;
 pub(crate) mod function_inserter;
 pub mod instruction;
+pub(crate) mod integer;
 pub mod map;
 pub(crate) mod post_order;
 pub(crate) mod printer;

compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs

@@ -59,8 +59,9 @@ use crate::ssa::{
         function_inserter::FunctionInserter,
         instruction::{
             Binary, BinaryOp, ConstrainError, Instruction, InstructionId,
-            binary::{convert_signed_integer_to_field_element, eval_constant_binary_op},
+            binary::eval_constant_binary_op,
         },
+        integer::IntegerConstant,
         post_order::PostOrder,
         types::{NumericType, Type},
         value::{Value, ValueId},
@@ -152,11 +153,11 @@ struct LoopInvariantContext<'f> {
     // This map is expected to only ever contain a singular value.
     // However, we store it in a map in order to match the definition of
     // `outer_induction_variables` as both maps share checks for evaluating binary operations.
-    current_induction_variables: HashMap<ValueId, (FieldElement, FieldElement)>,
+    current_induction_variables: HashMap<ValueId, (IntegerConstant, IntegerConstant)>,
     // Maps outer loop induction variable -> fixed lower and upper loop bound
     // This will be used by inner loops to determine whether they
     // have safe operations reliant upon an outer loop's maximum induction variable.
-    outer_induction_variables: HashMap<ValueId, (FieldElement, FieldElement)>,
+    outer_induction_variables: HashMap<ValueId, (IntegerConstant, IntegerConstant)>,
     // This context struct processes runs across all loops.
     // This stores the current loop's pre-header block.
     // It is wrapped in an Option as our SSA `Id<T>` does not allow dummy values.
@@ -368,9 +369,6 @@ impl<'f> LoopInvariantContext<'f> {
     fn set_induction_var_bounds(&mut self, loop_: &Loop, current_loop: bool) {
         let bounds = loop_.get_const_bounds(self.inserter.function, self.pre_header());
         if let Some((lower_bound, upper_bound)) = bounds {
-            let bit_size = NumericType::length_type().bit_size();
-            let lower_bound = convert_signed_integer_to_field_element(lower_bound, bit_size);
-            let upper_bound = convert_signed_integer_to_field_element(upper_bound, bit_size);
             let induction_variable = loop_.get_induction_variable(self.inserter.function);
             let induction_variable = self.inserter.resolve(induction_variable);
             if current_loop {
@@ -399,7 +397,7 @@ impl<'f> LoopInvariantContext<'f> {
                 let array_typ = self.inserter.function.dfg.type_of_value(*array);
                 let upper_bound = self.outer_induction_variables.get(index).map(|bounds| bounds.1);
                 if let (Type::Array(_, len), Some(upper_bound)) = (array_typ, upper_bound) {
-                    upper_bound.to_u128() <= len.into()
+                    upper_bound.apply(|i| i <= len.into(), |i| i <= len.into())
                 } else {
                     false
                 }
@@ -411,7 +409,9 @@ impl<'f> LoopInvariantContext<'f> {
                 // If the instruction were to be hoisted out of a loop that never executes it could potentially cause the program to fail when it is not meant to fail.
                 let bounds = self.current_induction_variables.values().next().copied();
                 let does_loop_body_execute = bounds
-                    .map(|(lower_bound, upper_bound)| !(upper_bound - lower_bound).is_zero())
+                    .and_then(|(lower_bound, upper_bound)| {
+                        upper_bound.reduce(lower_bound, |u, l| u > l, |u, l| u > l)
+                    })
                     .unwrap_or(false);
                 // If we know the loop will be executed these instructions can still only be hoisted if the instructions
                 // are in a non control dependent block.
@@ -576,13 +576,13 @@ impl<'f> LoopInvariantContext<'f> {
             _ => None,
         }?;
 
-        let min_iter = self.inserter.function.dfg.make_constant(*lower, NumericType::length_type());
-        assert!(*upper != FieldElement::zero(), "executing a non executable loop");
-        let max_iter = self
-            .inserter
-            .function
-            .dfg
-            .make_constant(*upper - FieldElement::one(), NumericType::length_type());
+        assert!(!upper.is_zero(), "executing a non executable loop");
+
+        let (upper_field, upper_type) = upper.dec().into_numeric_constant();
+        let (lower_field, lower_type) = lower.into_numeric_constant();
+
+        let min_iter = self.inserter.function.dfg.make_constant(lower_field, lower_type);
+        let max_iter = self.inserter.function.dfg.make_constant(upper_field, upper_type);
         if (is_left && self.is_loop_invariant(rhs)) || (!is_left && self.is_loop_invariant(lhs)) {
             return Some((is_left, min_iter, max_iter));
         }
@@ -636,9 +636,9 @@ impl<'f> LoopInvariantContext<'f> {
         lhs: &ValueId,
         rhs: &ValueId,
         only_outer_induction: bool,
-    ) -> Option<(bool, FieldElement, FieldElement, FieldElement)> {
-        let lhs_const = self.inserter.function.dfg.get_numeric_constant_with_type(*lhs);
-        let rhs_const = self.inserter.function.dfg.get_numeric_constant_with_type(*rhs);
+    ) -> Option<(bool, IntegerConstant, IntegerConstant, IntegerConstant)> {
+        let lhs_const = self.inserter.function.dfg.get_integer_constant(*lhs);
+        let rhs_const = self.inserter.function.dfg.get_integer_constant(*rhs);
         match (
             lhs_const,
             rhs_const,
@@ -651,10 +651,10 @@ impl<'f> LoopInvariantContext<'f> {
                 .and_then(|v| if only_outer_induction { None } else { Some(v) })
                 .or(self.outer_induction_variables.get(rhs)),
         ) {
-            (Some((lhs, _)), None, None, Some((lower_bound, upper_bound))) => {
+            (Some(lhs), None, None, Some((lower_bound, upper_bound))) => {
                 Some((false, lhs, *lower_bound, *upper_bound))
             }
-            (None, Some((rhs, _)), Some((lower_bound, upper_bound)), None) => {
+            (None, Some(rhs), Some((lower_bound, upper_bound)), None) => {
                 Some((true, rhs, *lower_bound, *upper_bound))
             }
             _ => None,
@@ -707,6 +707,8 @@ impl<'f> LoopInvariantContext<'f> {
             // of its inputs to check whether it will ever overflow.
             // If so, this will cause `eval_constant_binary_op` to return `None`.
             // Therefore a `Some` value shows that this operation is safe.
+            let lhs = lhs.into_numeric_constant().0;
+            let rhs = rhs.into_numeric_constant().0;
             if eval_constant_binary_op(lhs, rhs, binary.operator, operand_type).is_some() {
                 // Unchecked version of the binary operation
                 let unchecked = Instruction::Binary(Binary {
@@ -733,7 +735,7 @@ impl<'f> LoopInvariantContext<'f> {
                 true if upper_bound <= value => SimplifyResult::SimplifiedTo(self.true_value),
                 true if lower_bound >= value => SimplifyResult::SimplifiedTo(self.false_value),
                 false if lower_bound > value => SimplifyResult::SimplifiedTo(self.true_value),
-                false if upper_bound <= value + FieldElement::one() => {
+                false if upper_bound <= value.inc() => {
                     SimplifyResult::SimplifiedTo(self.false_value)
                 }
                 _ => SimplifyResult::None,
@@ -760,10 +762,10 @@ impl<'f> LoopInvariantContext<'f> {
                     return false;
                 };
                 if left {
-                    if value != FieldElement::zero() {
+                    if !value.is_zero() {
                         return true;
                     }
-                } else if lower != FieldElement::zero() {
+                } else if !lower.is_zero() {
                     return true;
                 }