Skip to content

Commit

Permalink
chore: enum dummy ID for FuncId in monomorphizer and docstring fixes (
Browse files Browse the repository at this point in the history
noir-lang#5421)

# Description

## Problem\*

Small issues or typos encountered during
noir-lang#5361

## Summary\*



## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: Maxim Vezenov <[email protected]>
Co-authored-by: jfecher <[email protected]>
  • Loading branch information
3 people authored Aug 26, 2024
1 parent ad69c54 commit 1a9a663
Show file tree
Hide file tree
Showing 18 changed files with 74 additions and 49 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -725,7 +725,7 @@ impl<'block> BrilligBlock<'block> {
1,
);
}
Instruction::EnableSideEffects { .. } => {
Instruction::EnableSideEffectsIf { .. } => {
todo!("enable_side_effects not supported by brillig")
}
Instruction::IfElse { .. } => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ pub(crate) fn directive_invert<F: AcirField>() -> GeneratedBrillig<F> {
///
/// This is equivalent to the Noir (pseudo)code
///
/// ```ignore
/// ```text
/// fn quotient<T>(a: T, b: T) -> (T,T) {
/// (a/b, a-a/b*b)
/// }
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ pub(crate) fn optimize_into_acir(
.run_pass(Ssa::inline_functions_with_no_predicates, "After Inlining:")
.run_pass(Ssa::remove_if_else, "After Remove IfElse:")
.run_pass(Ssa::fold_constants, "After Constant Folding:")
.run_pass(Ssa::remove_enable_side_effects, "After EnableSideEffects removal:")
.run_pass(Ssa::remove_enable_side_effects, "After EnableSideEffectsIf removal:")
.run_pass(Ssa::fold_constants_using_constraints, "After Constraint Folding:")
.run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:")
.run_pass(Ssa::array_set_optimization, "After Array Set Optimizations:")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,7 @@ impl<F: AcirField> AcirContext<F> {
self.sub_var(sum, mul)
} else {
// Implement OR in terms of AND
// (NOT a) NAND (NOT b) => a OR b
// (NOT a) AND (NOT b) => NOT (a OR b)
let a = self.not_var(lhs, typ.clone())?;
let b = self.not_var(rhs, typ.clone())?;
let a_and_b = self.and_var(a, b, typ.clone())?;
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,7 @@ impl<'a> Context<'a> {
self.convert_ssa_truncate(*value, *bit_size, *max_bit_size, dfg)?;
self.define_result_var(dfg, instruction_id, result_acir_var);
}
Instruction::EnableSideEffects { condition } => {
Instruction::EnableSideEffectsIf { condition } => {
let acir_var = self.convert_numeric_value(*condition, dfg)?;
self.current_side_effects_enabled_var = acir_var;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ impl Context {
}
Instruction::Allocate { .. }
| Instruction::DecrementRc { .. }
| Instruction::EnableSideEffects { .. }
| Instruction::EnableSideEffectsIf { .. }
| Instruction::IncrementRc { .. }
| Instruction::RangeCheck { .. } => {}
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa/function_builder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ impl FunctionBuilder {
/// Insert an enable_side_effects_if instruction. These are normally only automatically
/// inserted during the flattening pass when branching is removed.
pub(crate) fn insert_enable_side_effects_if(&mut self, condition: ValueId) {
self.insert_instruction(Instruction::EnableSideEffects { condition }, None);
self.insert_instruction(Instruction::EnableSideEffectsIf { condition }, None);
}

/// Terminates the current block with the given terminator instruction
Expand Down
45 changes: 33 additions & 12 deletions compiler/noirc_evaluator/src/ssa/ir/instruction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -218,13 +218,23 @@ pub(crate) enum Instruction {
Store { address: ValueId, value: ValueId },

/// Provides a context for all instructions that follow up until the next
/// `EnableSideEffects` is encountered, for stating a condition that determines whether
/// `EnableSideEffectsIf` is encountered, for stating a condition that determines whether
/// such instructions are allowed to have side-effects.
///
/// For example,
/// ```text
/// EnableSideEffectsIf condition0;
/// code0;
/// EnableSideEffectsIf condition1;
/// code1;
/// ```
/// - `code0` will have side effects iff `condition0` evaluates to `true`
/// - `code1` will have side effects iff `condition1` evaluates to `true`
///
/// This instruction is only emitted after the cfg flattening pass, and is used to annotate
/// instruction regions with an condition that corresponds to their position in the CFG's
/// if-branching structure.
EnableSideEffects { condition: ValueId },
EnableSideEffectsIf { condition: ValueId },

/// Retrieve a value from an array at the given index
ArrayGet { array: ValueId, index: ValueId },
Expand All @@ -249,6 +259,17 @@ pub(crate) enum Instruction {
DecrementRc { value: ValueId },

/// Merge two values returned from opposite branches of a conditional into one.
///
/// ```text
/// if then_condition {
/// then_value
/// } else { // else_condition = !then_condition
/// else_value
/// }
/// ```
///
/// Where we save the result of !then_condition so that we have the same
/// ValueId for it each time.
IfElse {
then_condition: ValueId,
then_value: ValueId,
Expand Down Expand Up @@ -279,7 +300,7 @@ impl Instruction {
| Instruction::IncrementRc { .. }
| Instruction::DecrementRc { .. }
| Instruction::RangeCheck { .. }
| Instruction::EnableSideEffects { .. } => InstructionResultType::None,
| Instruction::EnableSideEffectsIf { .. } => InstructionResultType::None,
Instruction::Allocate { .. }
| Instruction::Load { .. }
| Instruction::ArrayGet { .. }
Expand All @@ -306,7 +327,7 @@ impl Instruction {

match self {
// These either have side-effects or interact with memory
EnableSideEffects { .. }
EnableSideEffectsIf { .. }
| Allocate
| Load { .. }
| Store { .. }
Expand Down Expand Up @@ -362,7 +383,7 @@ impl Instruction {

Constrain(..)
| Store { .. }
| EnableSideEffects { .. }
| EnableSideEffectsIf { .. }
| IncrementRc { .. }
| DecrementRc { .. }
| RangeCheck { .. } => false,
Expand Down Expand Up @@ -401,7 +422,7 @@ impl Instruction {
!dfg.is_safe_index(*index, *array)
}

Instruction::EnableSideEffects { .. } | Instruction::ArraySet { .. } => true,
Instruction::EnableSideEffectsIf { .. } | Instruction::ArraySet { .. } => true,

Instruction::Call { func, .. } => match dfg[*func] {
Value::Function(_) => true,
Expand Down Expand Up @@ -466,8 +487,8 @@ impl Instruction {
Instruction::Store { address, value } => {
Instruction::Store { address: f(*address), value: f(*value) }
}
Instruction::EnableSideEffects { condition } => {
Instruction::EnableSideEffects { condition: f(*condition) }
Instruction::EnableSideEffectsIf { condition } => {
Instruction::EnableSideEffectsIf { condition: f(*condition) }
}
Instruction::ArrayGet { array, index } => {
Instruction::ArrayGet { array: f(*array), index: f(*index) }
Expand Down Expand Up @@ -541,7 +562,7 @@ impl Instruction {
f(*index);
f(*value);
}
Instruction::EnableSideEffects { condition } => {
Instruction::EnableSideEffectsIf { condition } => {
f(*condition);
}
Instruction::IncrementRc { value }
Expand Down Expand Up @@ -678,11 +699,11 @@ impl Instruction {
Instruction::Call { func, arguments } => {
simplify_call(*func, arguments, dfg, block, ctrl_typevars, call_stack)
}
Instruction::EnableSideEffects { condition } => {
Instruction::EnableSideEffectsIf { condition } => {
if let Some(last) = dfg[block].instructions().last().copied() {
let last = &mut dfg[last];
if matches!(last, Instruction::EnableSideEffects { .. }) {
*last = Instruction::EnableSideEffects { condition: *condition };
if matches!(last, Instruction::EnableSideEffectsIf { .. }) {
*last = Instruction::EnableSideEffectsIf { condition: *condition };
return Remove;
}
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa/ir/printer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ fn display_instruction_inner(
Instruction::Store { address, value } => {
writeln!(f, "store {} at {}", show(*value), show(*address))
}
Instruction::EnableSideEffects { condition } => {
Instruction::EnableSideEffectsIf { condition } => {
writeln!(f, "enable_side_effects {}", show(*condition))
}
Instruction::ArrayGet { array, index } => {
Expand Down
4 changes: 2 additions & 2 deletions compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,9 +159,9 @@ impl Context {
*side_effects_enabled_var,
);

// If we just inserted an `Instruction::EnableSideEffects`, we need to update `side_effects_enabled_var`
// If we just inserted an `Instruction::EnableSideEffectsIf`, we need to update `side_effects_enabled_var`
// so that we use the correct set of constrained values in future.
if let Instruction::EnableSideEffects { condition } = instruction {
if let Instruction::EnableSideEffectsIf { condition } = instruction {
*side_effects_enabled_var = condition;
};
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa/opt/die.rs
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ impl Context {
let instruction_id = *instruction_id;
let instruction = &function.dfg[instruction_id];

if let Instruction::EnableSideEffects { condition } = instruction {
if let Instruction::EnableSideEffectsIf { condition } = instruction {
side_effects_condition = Some(*condition);

// We still need to keep the EnableSideEffects instruction
Expand Down
6 changes: 3 additions & 3 deletions compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
//! elimination (DIE) pass.
//!
//! Though CFG information is lost during this pass, some key information is retained in the form
//! of `EnableSideEffect` instructions. Each time the flattening pass enters and exits a branch of
//! of `EnableSideEffectsIf` instructions. Each time the flattening pass enters and exits a branch of
//! a jmpif, an instruction is inserted to capture a condition that is analogous to the activeness
//! of the program point. For example:
//!
Expand Down Expand Up @@ -573,7 +573,7 @@ impl<'f> Context<'f> {
}

/// Checks the branch condition on the top of the stack and uses it to build and insert an
/// `EnableSideEffects` instruction into the entry block.
/// `EnableSideEffectsIf` instruction into the entry block.
///
/// If the stack is empty, a "true" u1 constant is taken to be the active condition. This is
/// necessary for re-enabling side-effects when re-emerging to a branch depth of 0.
Expand All @@ -584,7 +584,7 @@ impl<'f> Context<'f> {
self.inserter.function.dfg.make_constant(FieldElement::one(), Type::unsigned(1))
}
};
let enable_side_effects = Instruction::EnableSideEffects { condition };
let enable_side_effects = Instruction::EnableSideEffectsIf { condition };
let call_stack = self.inserter.function.dfg.get_value_call_stack(condition);
self.insert_instruction_with_typevars(enable_side_effects, None, call_stack);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ impl<'a> ValueMerger<'a> {
for (index, element_type, condition) in changed_indices {
let typevars = Some(vec![element_type.clone()]);

let instruction = Instruction::EnableSideEffects { condition };
let instruction = Instruction::EnableSideEffectsIf { condition };
self.insert_instruction(instruction);

let mut get_element = |array, typevars| {
Expand All @@ -398,7 +398,7 @@ impl<'a> ValueMerger<'a> {
array = self.insert_array_set(array, index, value, Some(condition)).first();
}

let instruction = Instruction::EnableSideEffects { condition: current_condition };
let instruction = Instruction::EnableSideEffectsIf { condition: current_condition };
self.insert_instruction(instruction);
Some(array)
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa/opt/inlining.rs
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,7 @@ impl<'function> PerFunctionContext<'function> {
}
None => self.push_instruction(*id),
},
Instruction::EnableSideEffects { condition } => {
Instruction::EnableSideEffectsIf { condition } => {
side_effects_enabled = Some(self.translate_value(*condition));
self.push_instruction(*id);
}
Expand Down
20 changes: 10 additions & 10 deletions compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
//! The goal of the "remove enable side effects" optimization pass is to delay any [Instruction::EnableSideEffects]
//! The goal of the "remove enable side effects" optimization pass is to delay any [Instruction::EnableSideEffectsIf]
//! instructions such that they cover the minimum number of instructions possible.
//!
//! The pass works as follows:
//! - Insert instructions until an [Instruction::EnableSideEffects] is encountered, save this [InstructionId].
//! - Insert instructions until an [Instruction::EnableSideEffectsIf] is encountered, save this [InstructionId].
//! - Continue inserting instructions until either
//! - Another [Instruction::EnableSideEffects] is encountered, if so then drop the previous [InstructionId] in favour
//! - Another [Instruction::EnableSideEffectsIf] is encountered, if so then drop the previous [InstructionId] in favour
//! of this one.
//! - An [Instruction] with side-effects is encountered, if so then insert the currently saved [Instruction::EnableSideEffects]
//! before the [Instruction]. Continue inserting instructions until the next [Instruction::EnableSideEffects] is encountered.
//! - An [Instruction] with side-effects is encountered, if so then insert the currently saved [Instruction::EnableSideEffectsIf]
//! before the [Instruction]. Continue inserting instructions until the next [Instruction::EnableSideEffectsIf] is encountered.
use std::collections::HashSet;

use acvm::{acir::AcirField, FieldElement};
Expand Down Expand Up @@ -70,10 +70,10 @@ impl Context {
for instruction_id in instructions {
let instruction = &function.dfg[instruction_id];

// If we run into another `Instruction::EnableSideEffects` before encountering any
// If we run into another `Instruction::EnableSideEffectsIf` before encountering any
// instructions with side effects then we can drop the instruction we're holding and
// continue with the new `Instruction::EnableSideEffects`.
if let Instruction::EnableSideEffects { condition } = instruction {
// continue with the new `Instruction::EnableSideEffectsIf`.
if let Instruction::EnableSideEffectsIf { condition } = instruction {
// If this instruction isn't changing the currently active condition then we can ignore it.
if active_condition == *condition {
continue;
Expand All @@ -98,7 +98,7 @@ impl Context {
}

// If we hit an instruction which is affected by the side effects var then we must insert the
// `Instruction::EnableSideEffects` before we insert this new instruction.
// `Instruction::EnableSideEffectsIf` before we insert this new instruction.
if Self::responds_to_side_effects_var(&function.dfg, instruction) {
if let Some(enable_side_effects_instruction_id) =
last_side_effects_enabled_instruction.take()
Expand Down Expand Up @@ -140,7 +140,7 @@ impl Context {
| IncrementRc { .. }
| DecrementRc { .. } => false,

EnableSideEffects { .. }
EnableSideEffectsIf { .. }
| ArrayGet { .. }
| ArraySet { .. }
| Allocate
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ impl Context {
self.slice_sizes.insert(result, old_capacity);
function.dfg[block].instructions_mut().push(instruction);
}
Instruction::EnableSideEffects { condition } => {
Instruction::EnableSideEffectsIf { condition } => {
current_conditional = *condition;
function.dfg[block].instructions_mut().push(instruction);
}
Expand Down
18 changes: 11 additions & 7 deletions compiler/noirc_frontend/src/monomorphization/ast.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::fmt::Display;

use acvm::FieldElement;
use iter_extended::vecmap;
use noirc_errors::{
Expand Down Expand Up @@ -67,6 +69,12 @@ pub struct LocalId(pub u32);
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
pub struct FuncId(pub u32);

impl Display for FuncId {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(f, "{}", self.0)
}
}

#[derive(Debug, Clone, Hash)]
pub struct Ident {
pub location: Option<Location>,
Expand Down Expand Up @@ -357,16 +365,12 @@ impl Program {
FuncId(0)
}

pub fn take_main_body(&mut self) -> Expression {
self.take_function_body(FuncId(0))
}

/// Takes a function body by replacing it with `false` and
/// returning the previous value
pub fn take_function_body(&mut self, function: FuncId) -> Expression {
let main = &mut self.functions[function.0 as usize];
let replacement = Expression::Literal(Literal::Bool(false));
std::mem::replace(&mut main.body, replacement)
let function_definition = &mut self[function];
let replacement = Expression::Block(vec![]);
std::mem::replace(&mut function_definition.body, replacement)
}
}

Expand Down
4 changes: 2 additions & 2 deletions compiler/noirc_frontend/src/monomorphization/printer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ impl AstPrinter {
write!(
f,
"fn {}$f{}({}) -> {} {{",
function.name, function.id.0, params, function.return_type
function.name, function.id, params, function.return_type
)?;
self.indent_level += 1;
self.print_expr_expect_block(&function.body, f)?;
Expand Down Expand Up @@ -291,7 +291,7 @@ impl Display for Definition {
fn fmt(&self, f: &mut Formatter) -> std::fmt::Result {
match self {
Definition::Local(id) => write!(f, "l{}", id.0),
Definition::Function(id) => write!(f, "f{}", id.0),
Definition::Function(id) => write!(f, "f{}", id),
Definition::Builtin(name) => write!(f, "{name}"),
Definition::LowLevel(name) => write!(f, "{name}"),
Definition::Oracle(name) => write!(f, "{name}"),
Expand Down

0 comments on commit 1a9a663

Please sign in to comment.