Skip to content

Commit

Permalink
chore: idempotent constant array creation
Browse files Browse the repository at this point in the history
  • Loading branch information
TomAFrench committed Aug 27, 2024
1 parent a5bab59 commit 0907b34
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 9 deletions.
10 changes: 9 additions & 1 deletion compiler/noirc_evaluator/src/ssa/ir/dfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ pub(crate) struct DataFlowGraph {
#[serde(skip)]
constants: HashMap<(FieldElement, Type), ValueId>,

#[serde(skip)]
arrays: HashMap<(im::Vector<ValueId>, Type), ValueId>,

/// Contains each function that has been imported into the current function.
/// A unique `ValueId` for each function's [`Value::Function`] is stored so any given FunctionId
/// will always have the same ValueId within this function.
Expand Down Expand Up @@ -268,7 +271,12 @@ impl DataFlowGraph {
/// Create a new constant array value from the given elements
pub(crate) fn make_array(&mut self, array: im::Vector<ValueId>, typ: Type) -> ValueId {
assert!(matches!(typ, Type::Array(..) | Type::Slice(_)));
self.make_value(Value::Array { array, typ })
if let Some(id) = self.arrays.get(&(array.clone(), typ.clone())) {
return *id;
}
let id = self.make_value(Value::Array { array: array.clone(), typ: typ.clone() });
self.arrays.insert((array, typ), id);
id
}

/// Gets or creates a ValueId for the given FunctionId.
Expand Down
6 changes: 4 additions & 2 deletions compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,10 @@ impl<'f> FunctionInserter<'f> {

let new_array_clone = new_array.clone();
let new_id = self.function.dfg.make_array(new_array, typ.clone());
self.values.insert(value, new_id);
self.const_arrays.insert((new_array_clone, typ), new_id);
if value != new_id {
self.values.insert(value, new_id);
self.const_arrays.insert((new_array_clone, typ), new_id);
}
new_id
}
_ => value,
Expand Down
35 changes: 29 additions & 6 deletions compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,18 @@
//! different blocks are merged, i.e. after the [`flatten_cfg`][super::flatten_cfg] pass.
use std::collections::HashSet;

use acvm::{acir::AcirField, FieldElement};
use acvm::{
acir::{AcirField, BlackBoxFunc},
FieldElement,
};
use iter_extended::vecmap;

use crate::ssa::{
ir::{
basic_block::BasicBlockId,
dfg::{DataFlowGraph, InsertInstructionResult},
function::Function,
instruction::{Instruction, InstructionId},
instruction::{Instruction, InstructionId, Intrinsic},
types::Type,
value::{Value, ValueId},
},
Expand Down Expand Up @@ -137,6 +140,15 @@ impl Context {
let instruction = Self::resolve_instruction(id, dfg, constraint_simplification_mapping);
let old_results = dfg.instruction_results(id).to_vec();

let keccakf1600 = dfg.import_intrinsic(Intrinsic::BlackBox(BlackBoxFunc::Keccakf1600));
match instruction {
Instruction::Call { ref func, ref arguments } if *func == keccakf1600 => {
println!("{instruction:?}");
println!("{:?}", dfg.get_array_constant(arguments[0]));
}
_ => (),
}

// If a copy of this instruction exists earlier in the block, then reuse the previous results.
if let Some(cached_results) =
Self::get_cached(dfg, instruction_result_cache, &instruction, *side_effects_enabled_var)
Expand Down Expand Up @@ -169,7 +181,7 @@ impl Context {
/// Fetches an [`Instruction`] by its [`InstructionId`] and fully resolves its inputs.
fn resolve_instruction(
instruction_id: InstructionId,
dfg: &DataFlowGraph,
dfg: &mut DataFlowGraph,
constraint_simplification_mapping: &HashMap<ValueId, ValueId>,
) -> Instruction {
let instruction = dfg[instruction_id].clone();
Expand All @@ -180,14 +192,23 @@ impl Context {
// This allows us to reach a stable final `ValueId` for each instruction input as we add more
// constraints to the cache.
fn resolve_cache(
dfg: &DataFlowGraph,
dfg: &mut DataFlowGraph,
cache: &HashMap<ValueId, ValueId>,
value_id: ValueId,
) -> ValueId {
let resolved_id = dfg.resolve(value_id);
match cache.get(&resolved_id) {
let resolved_id = match cache.get(&resolved_id) {
Some(cached_value) => resolve_cache(dfg, cache, *cached_value),
None => resolved_id,
};

// There's gotta be a better way
if let Some((array_contents, typ)) = dfg.get_array_constant(resolved_id) {
let resolved_array =
array_contents.into_iter().map(|val| resolve_cache(dfg, cache, val)).collect();
dfg.make_array(resolved_array, typ)
} else {
resolved_id
}
}

Expand Down Expand Up @@ -868,6 +889,8 @@ mod test {
let array1 = builder.array_constant(array_contents.clone().into(), typ.clone());
let array2 = builder.array_constant(array_contents.into(), typ.clone());

assert_eq!(array1, array2, "arrays were assigned different value ids");

let keccakf1600 =
builder.import_intrinsic("keccakf1600").expect("keccakf1600 intrinsic should exist");
let _v10 = builder.insert_call(keccakf1600, vec![array1], vec![typ.clone()]);
Expand All @@ -880,7 +903,7 @@ mod test {
let main = ssa.main();
let instructions = main.dfg[main.entry_block()].instructions();
let starting_instruction_count = instructions.len();
assert_eq!(starting_instruction_count, 2);
assert_eq!(starting_instruction_count, 3);

let ssa = ssa.fold_constants();

Expand Down

0 comments on commit 0907b34

Please sign in to comment.