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

feat(ssa): Globals DIE #7081

Merged
merged 8 commits into from
Jan 16, 2025
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,15 @@ impl<'block, 'global, Registers: RegisterAllocator> BrilligBlock<'block, 'global
brillig_block.convert_block(dfg);
}

pub(crate) fn compile_globals(&mut self, globals: &DataFlowGraph) {
pub(crate) fn compile_globals(
&mut self,
globals: &DataFlowGraph,
used_globals: &HashSet<ValueId>,
) {
for (id, value) in globals.values_iter() {
if !used_globals.contains(&id) {
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
continue;
}
match value {
Value::NumericConstant { .. } => {
self.convert_ssa_value(id, globals);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use acvm::FieldElement;
use fxhash::FxHashMap as HashMap;
use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet};

use super::{
BrilligArtifact, BrilligBlock, BrilligVariable, Function, FunctionContext, Label, ValueId,
Expand All @@ -9,6 +9,7 @@ use crate::brillig::{brillig_ir::BrilligContext, DataFlowGraph};
pub(crate) fn convert_ssa_globals(
enable_debug_trace: bool,
globals: &Function,
used_globals: &HashSet<ValueId>,
) -> (BrilligArtifact<FieldElement>, HashMap<ValueId, BrilligVariable>) {
let mut brillig_context = BrilligContext::new_for_global_init(enable_debug_trace);
// The global space does not have globals itself
Expand All @@ -28,7 +29,7 @@ pub(crate) fn convert_ssa_globals(
building_globals: true,
};

brillig_block.compile_globals(&globals.dfg);
brillig_block.compile_globals(&globals.dfg, used_globals);

brillig_context.return_instruction();

Expand Down
3 changes: 2 additions & 1 deletion compiler/noirc_evaluator/src/brillig/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ impl Ssa {

let mut brillig = Brillig::default();

let (artifact, brillig_globals) = convert_ssa_globals(enable_debug_trace, &self.globals);
let (artifact, brillig_globals) =
convert_ssa_globals(enable_debug_trace, &self.globals, &self.used_global_values);
brillig.globals = artifact;

for brillig_function_id in brillig_reachable_function_ids {
Expand Down
4 changes: 4 additions & 0 deletions compiler/noirc_evaluator/src/ssa/ir/dfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,10 @@ impl DataFlowGraph {
self.values.iter()
}

pub(crate) fn values_rev_iter(&self) -> impl ExactSizeIterator<Item = (ValueId, &Value)> {
self.values.rev_iter()
}

/// Returns the parameters of the given block
pub(crate) fn block_parameters(&self, block: BasicBlockId) -> &[ValueId] {
self.blocks[block].parameters()
Expand Down
5 changes: 5 additions & 0 deletions compiler/noirc_evaluator/src/ssa/ir/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,11 @@ impl<T> DenseMap<T> {
let ids_iter = (0..self.storage.len() as u32).map(|idx| Id::new(idx));
ids_iter.zip(self.storage.iter())
}

pub(crate) fn rev_iter(&self) -> impl ExactSizeIterator<Item = (Id<T>, &T)> {
let ids_iter = (0..self.storage.len() as u32).map(|idx| Id::new(idx));
ids_iter.zip(self.storage.iter()).rev()
}
}

impl<T> Default for DenseMap<T> {
Expand Down
45 changes: 36 additions & 9 deletions compiler/noirc_evaluator/src/ssa/opt/die.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,25 @@ impl Ssa {
/// unused results.
#[tracing::instrument(level = "trace", skip(self))]
pub(crate) fn dead_instruction_elimination(mut self) -> Ssa {
self.functions.par_iter_mut().for_each(|(_, func)| func.dead_instruction_elimination(true));
let mut used_global_values: HashSet<_> = self
.functions
.par_iter_mut()
.flat_map(|(_, func)| func.dead_instruction_elimination(true))
.collect();

// Check which globals are used across all functions
for (id, value) in self.globals.dfg.values_rev_iter() {
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
if used_global_values.contains(&id) {
if let Value::Instruction { instruction, .. } = &value {
let instruction = &self.globals.dfg[*instruction];
instruction.for_each_value(|value_id| {
used_global_values.insert(value_id);
});
}
}
}

self.used_global_values = used_global_values;

self
}
Expand All @@ -37,7 +55,13 @@ impl Function {
/// instructions that reference results from an instruction in another block are evaluated first.
/// If we did not iterate blocks in this order we could not safely say whether or not the results
/// of its instructions are needed elsewhere.
pub(crate) fn dead_instruction_elimination(&mut self, insert_out_of_bounds_checks: bool) {
///
/// Returns the set of globals that were used in this function.
/// After processing all functions, the union of these sets enables determining the unused globals.
pub(crate) fn dead_instruction_elimination(
&mut self,
insert_out_of_bounds_checks: bool,
) -> HashSet<ValueId> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add some explanation to the docstrings about the new return value? Are the globals the only reasonable thing to return here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add some comments. We do not need anything but the used globals as all other values are going to be per function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment

let mut context = Context::default();
for call_data in &self.dfg.data_bus.call_data {
context.mark_used_instruction_results(&self.dfg, call_data.array_id);
Expand All @@ -58,11 +82,12 @@ impl Function {
// instructions (we don't want to remove those checks, or instructions that are
// dependencies of those checks)
if inserted_out_of_bounds_checks {
self.dead_instruction_elimination(false);
return;
return self.dead_instruction_elimination(false);
}

context.remove_rc_instructions(&mut self.dfg);

context.used_values.into_iter().filter(|value| self.dfg.is_global(*value)).collect()
}
}

Expand Down Expand Up @@ -195,15 +220,17 @@ impl Context {
/// Inspects a value and marks all instruction results as used.
fn mark_used_instruction_results(&mut self, dfg: &DataFlowGraph, value_id: ValueId) {
let value_id = dfg.resolve(value_id);
if matches!(&dfg[value_id], Value::Instruction { .. } | Value::Param { .. }) {
if matches!(&dfg[value_id], Value::Instruction { .. } | Value::Param { .. })
|| dfg.is_global(value_id)
{
self.used_values.insert(value_id);
}
}

fn remove_rc_instructions(self, dfg: &mut DataFlowGraph) {
fn remove_rc_instructions(&self, dfg: &mut DataFlowGraph) {
let unused_rc_values_by_block: HashMap<BasicBlockId, HashSet<InstructionId>> =
self.rc_instructions.into_iter().fold(HashMap::default(), |mut acc, (rc, block)| {
let value = match &dfg[rc] {
self.rc_instructions.iter().fold(HashMap::default(), |mut acc, (rc, block)| {
let value = match &dfg[*rc] {
Instruction::IncrementRc { value } => *value,
Instruction::DecrementRc { value } => *value,
other => {
Expand All @@ -214,7 +241,7 @@ impl Context {
};

if !self.used_values.contains(&value) {
acc.entry(block).or_default().insert(rc);
acc.entry(*block).or_default().insert(*rc);
}
acc
});
Expand Down
5 changes: 4 additions & 1 deletion compiler/noirc_evaluator/src/ssa/opt/unrolling.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,10 @@

if has_unrolled {
if let Some((orig_function, max_incr_pct)) = orig_func_and_max_incr_pct {
let (_, brillig_globals) = convert_ssa_globals(false, &self.globals);
// DIE is run at the end of our SSA optimizations, so we mark all globals as in use here.
let used_globals = &self.globals.dfg.values_iter().map(|(id, _)| id).collect();
let (_, brillig_globals) =
convert_ssa_globals(false, &self.globals, used_globals);

let new_size = brillig_bytecode_size(function, &brillig_globals);
let orig_size = brillig_bytecode_size(&orig_function, &brillig_globals);
Expand Down Expand Up @@ -1024,7 +1027,7 @@
use super::{is_new_size_ok, BoilerplateStats, Loops};

/// Tries to unroll all loops in each SSA function once, calling the `Function` directly,
/// bypassing the iterative loop done by the SSA which does further optimisations.

Check warning on line 1030 in compiler/noirc_evaluator/src/ssa/opt/unrolling.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (optimisations)
///
/// If any loop cannot be unrolled, it is left as-is or in a partially unrolled state.
fn try_unroll_loops(mut ssa: Ssa) -> (Ssa, Vec<RuntimeError>) {
Expand Down
9 changes: 8 additions & 1 deletion compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::collections::BTreeMap;

use acvm::acir::circuit::ErrorSelector;
use fxhash::FxHashSet as HashSet;
use iter_extended::btree_map;
use serde::{Deserialize, Serialize};
use serde_with::serde_as;
Expand All @@ -11,13 +12,16 @@ use crate::ssa::ir::{
};
use noirc_frontend::hir_def::types::Type as HirType;

use super::ValueId;

/// Contains the entire SSA representation of the program.
#[serde_as]
#[derive(Serialize, Deserialize)]
pub(crate) struct Ssa {
#[serde_as(as = "Vec<(_, _)>")]
pub(crate) functions: BTreeMap<FunctionId, Function>,
pub(crate) globals: Function,
pub(crate) used_global_values: HashSet<ValueId>,
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
pub(crate) main_id: FunctionId,
#[serde(skip)]
pub(crate) next_id: AtomicCounter<Function>,
Expand Down Expand Up @@ -54,9 +58,12 @@ impl Ssa {
next_id: AtomicCounter::starting_after(max_id),
entry_point_to_generated_index: BTreeMap::new(),
error_selector_to_type: error_types,
// This field should be set afterwards as globals are generated
// These fields should be set afterwards as globals are generated
// outside of the FunctionBuilder, which is where the `Ssa` is instantiated.
globals: Function::new_for_globals(),
// This field is set only after running DIE and is utilized
// for optimizing implementation of globals post-SSA.
used_global_values: HashSet::default(),
}
}

Expand Down
Loading