Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: manage call stacks using a tree #6791

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/acir/acir_variable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use std::{borrow::Cow, hash::Hash};
use crate::brillig::brillig_ir::artifact::GeneratedBrillig;
use crate::errors::{InternalBug, InternalError, RuntimeError, SsaReport};
use crate::ssa::ir::{
dfg::CallStack, instruction::Endian, types::NumericType, types::Type as SsaType,
call_stack::CallStack, instruction::Endian, types::NumericType, types::Type as SsaType,
};

use super::big_int::BigIntContext;
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/acir/generated_acir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use super::brillig_directive;
use crate::{
brillig::brillig_ir::artifact::GeneratedBrillig,
errors::{InternalError, RuntimeError, SsaReport},
ssa::ir::dfg::CallStack,
ssa::ir::call_stack::CallStack,
ErrorType,
};

Expand Down
14 changes: 9 additions & 5 deletions compiler/noirc_evaluator/src/acir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ use crate::ssa::ir::instruction::Hint;
use crate::ssa::{
function_builder::data_bus::DataBus,
ir::{
dfg::{CallStack, DataFlowGraph},
call_stack::CallStack,
dfg::DataFlowGraph,
function::{Function, FunctionId, RuntimeType},
instruction::{
Binary, BinaryOp, ConstrainError, Instruction, InstructionId, Intrinsic,
Expand Down Expand Up @@ -675,7 +676,7 @@ impl<'a> Context<'a> {
brillig: &Brillig,
) -> Result<Vec<SsaReport>, RuntimeError> {
let instruction = &dfg[instruction_id];
self.acir_context.set_call_stack(dfg.get_call_stack(instruction_id));
self.acir_context.set_call_stack(dfg.get_instruction_call_stack(instruction_id));
let mut warnings = Vec::new();
match instruction {
Instruction::Binary(binary) => {
Expand Down Expand Up @@ -1822,7 +1823,7 @@ impl<'a> Context<'a> {
) -> Result<(Vec<AcirVar>, Vec<SsaReport>), RuntimeError> {
let (return_values, call_stack) = match terminator {
TerminatorInstruction::Return { return_values, call_stack } => {
(return_values, call_stack.clone())
(return_values, *call_stack)
}
// TODO(https://github.com/noir-lang/noir/issues/4616): Enable recursion on foldable/non-inlined ACIR functions
_ => unreachable!("ICE: Program must have a singular return"),
Expand All @@ -1841,6 +1842,7 @@ impl<'a> Context<'a> {
}
}

let call_stack = dfg.call_stack_data.get_call_stack(call_stack);
let warnings = if has_constant_return {
vec![SsaReport::Warning(InternalWarning::ReturnConstant { call_stack })]
} else {
Expand Down Expand Up @@ -2903,7 +2905,7 @@ mod test {
ssa::{
function_builder::FunctionBuilder,
ir::{
dfg::CallStack,
call_stack::CallStack,
function::FunctionId,
instruction::BinaryOp,
map::Id,
Expand Down Expand Up @@ -2932,7 +2934,9 @@ mod test {
// Set a call stack for testing whether `brillig_locations` in the `GeneratedAcir` was accurately set.
let mut stack = CallStack::unit(Location::dummy());
stack.push_back(Location::dummy());
builder.set_call_stack(stack);
let call_stack =
builder.current_function.dfg.call_stack_data.get_or_insert_locations(stack);
builder.set_call_stack(call_stack);

let foo_v0 = builder.add_parameter(Type::field());
let foo_v1 = builder.add_parameter(Type::field());
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/brillig/brillig_gen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use super::{
};
use crate::{
errors::InternalError,
ssa::ir::{dfg::CallStack, function::Function},
ssa::ir::{call_stack::CallStack, function::Function},
};

/// Converting an SSA function into Brillig bytecode.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::brillig::brillig_ir::registers::Stack;
use crate::brillig::brillig_ir::{
BrilligBinaryOp, BrilligContext, ReservedRegisters, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE,
};
use crate::ssa::ir::dfg::CallStack;
use crate::ssa::ir::call_stack::CallStack;
use crate::ssa::ir::instruction::{ConstrainError, Hint};
use crate::ssa::ir::{
basic_block::BasicBlockId,
Expand Down Expand Up @@ -201,7 +201,7 @@ impl<'block> BrilligBlock<'block> {
/// Converts an SSA instruction into a sequence of Brillig opcodes.
fn convert_ssa_instruction(&mut self, instruction_id: InstructionId, dfg: &DataFlowGraph) {
let instruction = &dfg[instruction_id];
self.brillig_context.set_call_stack(dfg.get_call_stack(instruction_id));
self.brillig_context.set_call_stack(dfg.get_instruction_call_stack(instruction_id));

self.initialize_constants(
&self.function_context.constant_allocation.allocated_at_location(
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/brillig/brillig_ir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ pub(crate) use instructions::BrilligBinaryOp;
use registers::{RegisterAllocator, ScratchSpace};

use self::{artifact::BrilligArtifact, debug_show::DebugToString, registers::Stack};
use crate::ssa::ir::dfg::CallStack;
use crate::ssa::ir::call_stack::CallStack;
use acvm::{
acir::brillig::{MemoryAddress, Opcode as BrilligOpcode},
AcirField,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use acvm::acir::brillig::Opcode as BrilligOpcode;
use acvm::acir::circuit::ErrorSelector;
use std::collections::{BTreeMap, HashMap};

use crate::ssa::ir::{basic_block::BasicBlockId, dfg::CallStack, function::FunctionId};
use crate::ssa::ir::{basic_block::BasicBlockId, call_stack::CallStack, function::FunctionId};
use crate::ErrorType;

use super::procedures::ProcedureId;
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use iter_extended::vecmap;
use noirc_errors::{CustomDiagnostic as Diagnostic, FileDiagnostic};
use thiserror::Error;

use crate::ssa::ir::{dfg::CallStack, types::NumericType};
use crate::ssa::ir::{call_stack::CallStack, types::NumericType};
use serde::{Deserialize, Serialize};

#[derive(Debug, PartialEq, Eq, Clone, Error)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ impl DependencyContext {
.keys()
.map(|brillig_call| {
SsaReport::Bug(InternalBug::UncheckedBrilligCall {
call_stack: function.dfg.get_call_stack(*brillig_call),
call_stack: function.dfg.get_instruction_call_stack(*brillig_call),
})
})
.collect();
Expand Down Expand Up @@ -513,7 +513,7 @@ impl Context {
// There is a value not in the set, which means that the inputs/outputs of this call have not been properly constrained
if unused_inputs {
warnings.push(SsaReport::Bug(InternalBug::IndependentSubgraph {
call_stack: function.dfg.get_call_stack(
call_stack: function.dfg.get_instruction_call_stack(
self.brillig_return_to_instruction_id[&brillig_output_in_set],
),
}));
Expand Down
28 changes: 16 additions & 12 deletions compiler/noirc_evaluator/src/ssa/function_builder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ use crate::ssa::ir::{
use super::{
ir::{
basic_block::BasicBlock,
dfg::{CallStack, InsertInstructionResult},
call_stack::{CallStack, CallStackId},
dfg::InsertInstructionResult,
function::RuntimeType,
instruction::{ConstrainError, InstructionId, Intrinsic},
types::NumericType,
Expand All @@ -34,10 +35,10 @@ use super::{
/// Contrary to the name, this struct has the capacity to build as many
/// functions as needed, although it is limited to one function at a time.
pub(crate) struct FunctionBuilder {
pub(super) current_function: Function,
pub(crate) current_function: Function,
current_block: BasicBlockId,
finished_functions: Vec<Function>,
call_stack: CallStack,
call_stack: CallStackId,
error_types: BTreeMap<ErrorSelector, HirType>,
}

Expand All @@ -53,7 +54,7 @@ impl FunctionBuilder {
current_block: new_function.entry_block(),
current_function: new_function,
finished_functions: Vec::new(),
call_stack: CallStack::new(),
call_stack: CallStackId::root(),
error_types: BTreeMap::default(),
}
}
Expand All @@ -78,11 +79,14 @@ impl FunctionBuilder {
function_id: FunctionId,
runtime_type: RuntimeType,
) {
let call_stack = self.current_function.dfg.get_call_stack(self.call_stack);
let mut new_function = Function::new(name, function_id);
new_function.set_runtime(runtime_type);
self.current_block = new_function.entry_block();

let old_function = std::mem::replace(&mut self.current_function, new_function);
// Copy the call stack to the new function
self.call_stack =
self.current_function.dfg.call_stack_data.get_or_insert_locations(call_stack);
self.finished_functions.push(old_function);
}

Expand Down Expand Up @@ -171,7 +175,7 @@ impl FunctionBuilder {
instruction,
block,
ctrl_typevars,
self.call_stack.clone(),
self.call_stack,
)
}

Expand All @@ -196,17 +200,17 @@ impl FunctionBuilder {
}

pub(crate) fn set_location(&mut self, location: Location) -> &mut FunctionBuilder {
self.call_stack = CallStack::unit(location);
self.call_stack = self.current_function.dfg.call_stack_data.add_location_to_root(location);
self
}

pub(crate) fn set_call_stack(&mut self, call_stack: CallStack) -> &mut FunctionBuilder {
pub(crate) fn set_call_stack(&mut self, call_stack: CallStackId) -> &mut FunctionBuilder {
self.call_stack = call_stack;
self
}

pub(crate) fn get_call_stack(&self) -> CallStack {
self.call_stack.clone()
self.current_function.dfg.get_call_stack(self.call_stack)
}

/// Insert a Load instruction at the end of the current block, loading from the given address
Expand Down Expand Up @@ -378,7 +382,7 @@ impl FunctionBuilder {
destination: BasicBlockId,
arguments: Vec<ValueId>,
) {
let call_stack = self.call_stack.clone();
let call_stack = self.call_stack;
self.terminate_block_with(TerminatorInstruction::Jmp {
destination,
arguments,
Expand All @@ -394,7 +398,7 @@ impl FunctionBuilder {
then_destination: BasicBlockId,
else_destination: BasicBlockId,
) {
let call_stack = self.call_stack.clone();
let call_stack = self.call_stack;
self.terminate_block_with(TerminatorInstruction::JmpIf {
condition,
then_destination,
Expand All @@ -405,7 +409,7 @@ impl FunctionBuilder {

/// Terminate the current block with a return instruction
pub(crate) fn terminate_with_return(&mut self, return_values: Vec<ValueId>) {
let call_stack = self.call_stack.clone();
let call_stack = self.call_stack;
self.terminate_block_with(TerminatorInstruction::Return { return_values, call_stack });
}

Expand Down
4 changes: 2 additions & 2 deletions compiler/noirc_evaluator/src/ssa/ir/basic_block.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::{
dfg::CallStack,
call_stack::CallStackId,
instruction::{InstructionId, TerminatorInstruction},
map::Id,
value::ValueId,
Expand Down Expand Up @@ -123,7 +123,7 @@ impl BasicBlock {
terminator,
TerminatorInstruction::Return {
return_values: Vec::new(),
call_stack: CallStack::new(),
call_stack: CallStackId::root(),
},
)
}
Expand Down
114 changes: 114 additions & 0 deletions compiler/noirc_evaluator/src/ssa/ir/call_stack.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
use serde::{Deserialize, Serialize};

use noirc_errors::Location;

pub(crate) type CallStack = im::Vector<Location>;

#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)]
pub(crate) struct CallStackId(u32);

impl CallStackId {
pub(crate) fn root() -> Self {
Self::new(0)
}

fn new(id: usize) -> Self {
Self(id as u32)
}

pub(crate) fn index(&self) -> usize {
self.0 as usize
}

pub(crate) fn is_root(&self) -> bool {
self.0 == 0
}
}

#[derive(Debug, Clone, Serialize, Deserialize)]
pub(crate) struct LocationNode {
pub(crate) parent: Option<CallStackId>,
pub(crate) children: Vec<CallStackId>,
pub(crate) value: Location,
}

#[derive(Debug, Default, Clone, Serialize, Deserialize)]
pub(crate) struct CallStackHelper {
locations: Vec<LocationNode>,
}

impl CallStackHelper {
/// Construct a CallStack from a CallStackId
pub(crate) fn get_call_stack(&self, mut call_stack: CallStackId) -> CallStack {
let mut result = im::Vector::new();
while let Some(parent) = self.locations[call_stack.index()].parent {
result.push_back(self.locations[call_stack.index()].value);
call_stack = parent;
}
result
}

/// Returns a new CallStackId which extends the call_stack with the provided call_stack.
pub(crate) fn extend_call_stack(
&mut self,
mut call_stack: CallStackId,
locations: &CallStack,
) -> CallStackId {
for location in locations {
call_stack = self.add_child(call_stack, *location);
}
call_stack
}

/// Adds a location to the call stack
pub(crate) fn add_child(&mut self, call_stack: CallStackId, location: Location) -> CallStackId {
if let Some(result) = self.locations[call_stack.index()]
.children
.iter()
.rev()
.take(1000)
Comment on lines +76 to +78
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we're searching in reverse so that we find recent additions faster; is there any significance as to why give up after 1000 children, after which a duplicate entry is acceptable? Is that value something that was actually observed?

I wonder if you thought some reverse index from e.g. the hash of the location to the child CallStackId could be used to make this faster and easier to reason about? Like children: HashMap<u64, CallStackId> where the key is fxhash::hash64(location).

.find(|child| self.locations[child.index()].value == location)
{
return *result;
}
self.locations.push(LocationNode {
parent: Some(call_stack),
children: vec![],
value: location,
});
let new_location = CallStackId::new(self.locations.len() - 1);
self.locations[call_stack.index()].children.push(new_location);
new_location
}

/// Retrieve the CallStackId corresponding to call_stack with the last 'len' locations removed.
pub(crate) fn unwind_call_stack(
&self,
mut call_stack: CallStackId,
mut len: usize,
) -> CallStackId {
while len > 0 {
if let Some(parent) = self.locations[call_stack.index()].parent {
len -= 1;
call_stack = parent;
} else {
break;
}
}
call_stack
}

pub(crate) fn add_location_to_root(&mut self, location: Location) -> CallStackId {
if self.locations.is_empty() {
self.locations.push(LocationNode { parent: None, children: vec![], value: location });
CallStackId::root()
} else {
self.add_child(CallStackId::root(), location)
}
}

/// Get (or create) a CallStackId corresponding to the given locations
pub(crate) fn get_or_insert_locations(&mut self, locations: CallStack) -> CallStackId {
self.extend_call_stack(CallStackId::root(), &locations)
}
}
Loading
Loading