chore(ssa refactor): Fix recursive printing of blocks (#1230)

* Implement ssa-gen for if

* Satisfy the clippy gods

* Fix printing bug

* Print constants directly

* chore(ssa refactor): Implement for loops (#1233)

Impl for loops

crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs

@@ -234,8 +234,17 @@ 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.
     pub(crate) fn get_numeric_constant(&self, value: Id<Value>) -> Option<FieldElement> {
+        self.get_numeric_constant_with_type(value).map(|(value, _typ)| value)
+    }
+
+    /// 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.
+    pub(crate) fn get_numeric_constant_with_type(
+        &self,
+        value: Id<Value>,
+    ) -> Option<(FieldElement, Type)> {
         match self.values[value] {
-            Value::NumericConstant { constant, .. } => Some(self[constant].value()),
+            Value::NumericConstant { constant, typ } => Some((self[constant].value(), typ)),
             _ => None,
         }
     }

crates/noirc_evaluator/src/ssa_refactor/ir/map.rs

@@ -69,9 +69,21 @@ impl<T> std::fmt::Debug for Id<T> {
     }
 }
 
-impl<T> std::fmt::Display for Id<T> {
+impl std::fmt::Display for Id<super::basic_block::BasicBlock> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "${}", self.index)
+        write!(f, "b{}", self.index)
+    }
+}
+
+impl std::fmt::Display for Id<super::value::Value> {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "v{}", self.index)
+    }
+}
+
+impl std::fmt::Display for Id<super::function::Function> {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "f{}", self.index)
     }
 }
 

crates/noirc_evaluator/src/ssa_refactor/ir/printer.rs

@@ -1,5 +1,8 @@
 //! This file is for pretty-printing the SSA IR in a human-readable form for debugging.
-use std::fmt::{Formatter, Result};
+use std::{
+    collections::HashSet,
+    fmt::{Formatter, Result},
+};
 
 use iter_extended::vecmap;
 
@@ -12,19 +15,26 @@ use super::{
 
 pub(crate) fn display_function(function: &Function, f: &mut Formatter) -> Result {
     writeln!(f, "fn {} {{", function.name)?;
-    display_block_with_successors(function, function.entry_block, f)?;
+    display_block_with_successors(function, function.entry_block, &mut HashSet::new(), f)?;
     write!(f, "}}")
 }
 
+/// Displays a block followed by all of its successors recursively.
+/// This uses a HashSet to keep track of the visited blocks. Otherwise,
+/// there would be infinite recursion for any loops in the IR.
 pub(crate) fn display_block_with_successors(
     function: &Function,
     block_id: BasicBlockId,
+    visited: &mut HashSet<BasicBlockId>,
     f: &mut Formatter,
 ) -> Result {
     display_block(function, block_id, f)?;
+    visited.insert(block_id);
 
     for successor in function.dfg[block_id].successors() {
-        display_block(function, successor, f)?;
+        if !visited.contains(&successor) {
+            display_block_with_successors(function, successor, visited, f)?;
+        }
     }
     Ok(())
 }
@@ -36,26 +46,36 @@ pub(crate) fn display_block(
 ) -> Result {
     let block = &function.dfg[block_id];
 
-    writeln!(f, "{}({}):", block_id, value_list(block.parameters()))?;
+    writeln!(f, "  {}({}):", block_id, value_list(function, block.parameters()))?;
 
     for instruction in block.instructions() {
         display_instruction(function, *instruction, f)?;
     }
 
-    display_terminator(block.terminator(), f)
+    display_terminator(function, block.terminator(), f)
+}
+
+/// Specialize displaying value ids so that if they refer to constants we
+/// print the constant directly
+fn value(function: &Function, id: ValueId) -> String {
+    match function.dfg.get_numeric_constant_with_type(id) {
+        Some((value, typ)) => format!("{} {}", value, typ),
+        None => id.to_string(),
+    }
 }
 
-fn value_list(values: &[ValueId]) -> String {
-    vecmap(values, ToString::to_string).join(", ")
+fn value_list(function: &Function, values: &[ValueId]) -> String {
+    vecmap(values, |id| value(function, *id)).join(", ")
 }
 
 pub(crate) fn display_terminator(
+    function: &Function,
     terminator: Option<&TerminatorInstruction>,
     f: &mut Formatter,
 ) -> Result {
     match terminator {
         Some(TerminatorInstruction::Jmp { destination, arguments }) => {
-            writeln!(f, "    jmp {}({})", destination, value_list(arguments))
+            writeln!(f, "    jmp {}({})", destination, value_list(function, arguments))
         }
         Some(TerminatorInstruction::JmpIf { condition, then_destination, else_destination }) => {
             writeln!(
@@ -65,7 +85,7 @@ pub(crate) fn display_terminator(
             )
         }
         Some(TerminatorInstruction::Return { return_values }) => {
-            writeln!(f, "    return {}", value_list(return_values))
+            writeln!(f, "    return {}", value_list(function, return_values))
         }
         None => writeln!(f, "    (no terminator instruction)"),
     }
@@ -81,29 +101,34 @@ pub(crate) fn display_instruction(
 
     let results = function.dfg.instruction_results(instruction);
     if !results.is_empty() {
-        write!(f, "{} = ", value_list(results))?;
+        write!(f, "{} = ", value_list(function, results))?;
     }
 
+    let show = |id| value(function, id);
+
     match &function.dfg[instruction] {
         Instruction::Binary(binary) => {
-            writeln!(f, "{} {}, {}", binary.operator, binary.lhs, binary.rhs)
+            writeln!(f, "{} {}, {}", binary.operator, show(binary.lhs), show(binary.rhs))
         }
-        Instruction::Cast(value, typ) => writeln!(f, "cast {value} as {typ}"),
-        Instruction::Not(value) => writeln!(f, "not {value}"),
+        Instruction::Cast(lhs, typ) => writeln!(f, "cast {} as {typ}", show(*lhs)),
+        Instruction::Not(rhs) => writeln!(f, "not {}", show(*rhs)),
         Instruction::Truncate { value, bit_size, max_bit_size } => {
-            writeln!(f, "truncate {value} to {bit_size} bits, max_bit_size: {max_bit_size}")
+            let value = show(*value);
+            writeln!(f, "truncate {value} to {bit_size} bits, max_bit_size: {max_bit_size}",)
         }
         Instruction::Constrain(value) => {
-            writeln!(f, "constrain {value}")
+            writeln!(f, "constrain {}", show(*value))
         }
         Instruction::Call { func, arguments } => {
-            writeln!(f, "call {func}({})", value_list(arguments))
+            writeln!(f, "call {func}({})", value_list(function, arguments))
         }
         Instruction::Intrinsic { func, arguments } => {
-            writeln!(f, "intrinsic {func}({})", value_list(arguments))
+            writeln!(f, "intrinsic {func}({})", value_list(function, arguments))
         }
         Instruction::Allocate { size } => writeln!(f, "alloc {size} fields"),
-        Instruction::Load { address } => writeln!(f, "load {address}"),
-        Instruction::Store { address, value } => writeln!(f, "store {value} at {address}"),
+        Instruction::Load { address } => writeln!(f, "load {}", show(*address)),
+        Instruction::Store { address, value } => {
+            writeln!(f, "store {} at {}", show(*address), show(*value))
+        }
     }
 }

crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs

@@ -164,6 +164,11 @@ impl<'a> FunctionContext<'a> {
         }
         address
     }
+
+    pub(super) fn define(&mut self, id: LocalId, value: Values) {
+        let existing = self.definitions.insert(id, value);
+        assert!(existing.is_none(), "Variable {id:?} was defined twice in ssa-gen pass");
+    }
 }
 
 /// True if the given operator cannot be encoded directly and needs

crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs

@@ -166,8 +166,34 @@ impl<'a> FunctionContext<'a> {
         self.builder.insert_cast(lhs, typ).into()
     }
 
-    fn codegen_for(&mut self, _for_expr: &ast::For) -> Values {
-        todo!()
+    fn codegen_for(&mut self, for_expr: &ast::For) -> Values {
+        let loop_entry = self.builder.insert_block();
+        let loop_body = self.builder.insert_block();
+        let loop_end = self.builder.insert_block();
+
+        // this is the 'i' in `for i in start .. end { block }`
+        let loop_index = self.builder.add_block_parameter(loop_entry, Type::field());
+
+        let start_index = self.codegen_non_tuple_expression(&for_expr.start_range);
+        let end_index = self.codegen_non_tuple_expression(&for_expr.end_range);
+
+        self.builder.terminate_with_jmp(loop_entry, vec![start_index]);
+
+        // Compile the loop entry block
+        self.builder.switch_to_block(loop_entry);
+        let jump_condition = self.builder.insert_binary(loop_index, BinaryOp::Lt, end_index);
+        self.builder.terminate_with_jmpif(jump_condition, loop_body, loop_end);
+
+        // Compile the loop body
+        self.builder.switch_to_block(loop_body);
+        self.define(for_expr.index_variable, loop_index.into());
+        self.codegen_expression(&for_expr.block);
+        let new_loop_index = self.make_offset(loop_index, 1);
+        self.builder.terminate_with_jmp(loop_entry, vec![new_loop_index]);
+
+        // Finish by switching back to the end of the loop
+        self.builder.switch_to_block(loop_end);
+        self.unit_value()
     }
 
     fn codegen_if(&mut self, if_expr: &ast::If) -> Values {

crates/noirc_frontend/src/parser/parser.rs

@@ -1238,46 +1238,46 @@ mod test {
         );
     }
 
-      #[test]
-      fn parse_assert() {
-          parse_with(assertion(expression()), "assert(x == y)").unwrap();
-  
-          // Currently we disallow constrain statements where the outer infix operator
-          // produces a value. This would require an implicit `==` which
-          // may not be intuitive to the user.
-          //
-          // If this is deemed useful, one would either apply a transformation
-          // or interpret it with an `==` in the evaluator
-          let disallowed_operators = vec![
-              BinaryOpKind::And,
-              BinaryOpKind::Subtract,
-              BinaryOpKind::Divide,
-              BinaryOpKind::Multiply,
-              BinaryOpKind::Or,
-          ];
-  
-          for operator in disallowed_operators {
-              let src = format!("assert(x {} y);", operator.as_string());
-              parse_with(assertion(expression()), &src).unwrap_err();
-          }
-  
-          // These are general cases which should always work.
-          //
-          // The first case is the most noteworthy. It contains two `==`
-          // The first (inner) `==` is a predicate which returns 0/1
-          // The outer layer is an infix `==` which is
-          // associated with the Constrain statement
-          parse_all(
-              assertion(expression()),
-              vec![
-                  "assert(((x + y) == k) + z == y)",
-                  "assert((x + !y) == y)",
-                  "assert((x ^ y) == y)",
-                  "assert((x ^ y) == (y + m))",
-                  "assert(x + x ^ x == y | m)",
-              ],
-          );
-      }
+    #[test]
+    fn parse_assert() {
+        parse_with(assertion(expression()), "assert(x == y)").unwrap();
+
+        // Currently we disallow constrain statements where the outer infix operator
+        // produces a value. This would require an implicit `==` which
+        // may not be intuitive to the user.
+        //
+        // If this is deemed useful, one would either apply a transformation
+        // or interpret it with an `==` in the evaluator
+        let disallowed_operators = vec![
+            BinaryOpKind::And,
+            BinaryOpKind::Subtract,
+            BinaryOpKind::Divide,
+            BinaryOpKind::Multiply,
+            BinaryOpKind::Or,
+        ];
+
+        for operator in disallowed_operators {
+            let src = format!("assert(x {} y);", operator.as_string());
+            parse_with(assertion(expression()), &src).unwrap_err();
+        }
+
+        // These are general cases which should always work.
+        //
+        // The first case is the most noteworthy. It contains two `==`
+        // The first (inner) `==` is a predicate which returns 0/1
+        // The outer layer is an infix `==` which is
+        // associated with the Constrain statement
+        parse_all(
+            assertion(expression()),
+            vec![
+                "assert(((x + y) == k) + z == y)",
+                "assert((x + !y) == y)",
+                "assert((x ^ y) == y)",
+                "assert((x ^ y) == (y + m))",
+                "assert(x + x ^ x == y | m)",
+            ],
+        );
+    }
 
     #[test]
     fn parse_let() {
@@ -1322,7 +1322,7 @@ mod test {
                 "fn f(f: pub Field, y : Field, z : comptime Field) -> u8 { x + a }",
                 "fn func_name(f: Field, y : pub Field, z : pub [u8;5],) {}",
                 "fn func_name(x: [Field], y : [Field;2],y : pub [Field;2], z : pub [u8;5])  {}",
-                "fn main(x: pub u8, y: pub u8) -> distinct pub [u8; 2] { [x, y] }"
+                "fn main(x: pub u8, y: pub u8) -> distinct pub [u8; 2] { [x, y] }",
             ],
         );