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

fix(ssa refactor): resolve replaced value ids for printing #1535

Merged
merged 3 commits into from
Jun 12, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 22 additions & 6 deletions crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,11 @@ pub(crate) struct DataFlowGraph {

/// All blocks in a function
blocks: DenseMap<BasicBlock>,

/// Debugging information about which `ValueId`s have had their underlying `Value` substituted
/// for that of another. This information is purely used for printing the SSA, and has no
/// material effect on the SSA itself.
replaced_value_ids: HashMap<ValueId, ValueId>,
}

impl DataFlowGraph {
Expand Down Expand Up @@ -146,21 +151,32 @@ impl DataFlowGraph {
self.values.insert(value)
}

/// Replaces the value specified by the given ValueId with a new Value.
/// Set the value of value_to_replace to refer to the value referred to by new_value.
///
/// This is the preferred method to call for optimizations simplifying
/// values since other instructions referring to the same ValueId need
/// not be modified to refer to a new ValueId.
pub(crate) fn set_value(&mut self, value_id: ValueId, new_value: Value) {
self.values[value_id] = new_value;
}

/// Set the value of value_to_replace to refer to the value referred to by new_value.
pub(crate) fn set_value_from_id(&mut self, value_to_replace: ValueId, new_value: ValueId) {
// Replaced `ValueId`s are tracked purely for debugging purposes.
// We first call `resolve_replaced_value_id(new_value)` in case `new_value` is also a
// `ValueId` that has previously had its corresponding `Value` substituted.
self.replaced_value_ids.insert(value_to_replace, self.resolve_replaced_value_id(new_value));

let new_value = self.values[new_value];
self.values[value_to_replace] = new_value;
}

/// If `original_value_id`'s underlying `Value` has been substituted for that of another
/// `ValueId`, this function will return the `ValueId` from which the substitution was taken.
/// If `original_value_id`'s underlying `Value` has not been substituted, the same `ValueId`
/// is returned.
///
/// The function exists purely for debugging purposes. Only the SSA printer is concerned with
/// this information.
pub(crate) fn resolve_replaced_value_id(&self, original_value_id: ValueId) -> ValueId {
*self.replaced_value_ids.get(&original_value_id).unwrap_or(&original_value_id)
}

/// Creates a new constant value, or returns the Id to an existing one if
/// one already exists.
pub(crate) fn make_constant(&mut self, constant: FieldElement, typ: Type) -> ValueId {
Expand Down
2 changes: 1 addition & 1 deletion crates/noirc_evaluator/src/ssa_refactor/ir/printer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ fn value(function: &Function, id: ValueId) -> String {
}
Value::Function(id) => id.to_string(),
Value::Intrinsic(intrinsic) => intrinsic.to_string(),
_ => id.to_string(),
_ => function.dfg.resolve_replaced_value_id(id).to_string(),
}
}

Expand Down
5 changes: 2 additions & 3 deletions crates/noirc_evaluator/src/ssa_refactor/opt/mem2reg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,7 @@ impl PerBlockContext {
Instruction::Load { address } => {
if let Some(address) = self.try_const_address(*address, dfg) {
if let Some(last_value) = self.last_stores.get(&address) {
let last_value = dfg[*last_value];
loads_to_substitute.push((*instruction_id, last_value));
loads_to_substitute.push((*instruction_id, *last_value));
} else {
self.failed_substitutes.insert(address);
}
Expand All @@ -128,7 +127,7 @@ impl PerBlockContext {
.instruction_results(*instruction_id)
.first()
.expect("ICE: Load instructions should have single result");
dfg.set_value(result_value, *new_value);
dfg.set_value_from_id(result_value, *new_value);
}

// Delete load instructions
Expand Down