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): Expand PR #1535 to resolve ValueIds in all SSA passes #1642

Merged
merged 3 commits into from
Jun 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
1 change: 1 addition & 0 deletions crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,7 @@ impl Context {
/// involving such values are evaluated via a separate path and stored in
/// `ssa_value_to_array_address` instead.
fn convert_value(&mut self, value_id: ValueId, dfg: &DataFlowGraph) -> AcirValue {
let value_id = dfg.resolve(value_id);
let value = &dfg[value_id];
if let Some(acir_value) = self.ssa_values.get(&value_id) {
return acir_value.clone();
Expand Down
22 changes: 10 additions & 12 deletions crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,24 +157,22 @@ impl DataFlowGraph {
/// values since other instructions referring to the same ValueId need
/// not be modified to refer to a new ValueId.
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].clone();
self.values[value_to_replace] = new_value;
if value_to_replace != new_value {
self.replaced_value_ids.insert(value_to_replace, self.resolve(new_value));
let new_value = self.values[new_value].clone();
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)
pub(crate) fn resolve(&self, original_value_id: ValueId) -> ValueId {
match self.replaced_value_ids.get(&original_value_id) {
Some(id) => self.resolve(*id),
None => original_value_id,
}
}

/// Creates a new constant value, or returns the Id to an existing one if
Expand Down
5 changes: 2 additions & 3 deletions crates/noirc_evaluator/src/ssa_refactor/ir/printer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ pub(crate) fn display_block(
/// constant or a function we print those directly.
fn value(function: &Function, id: ValueId) -> String {
use super::value::Value;
let id = function.dfg.resolve(id);
match &function.dfg[id] {
Value::NumericConstant { constant, typ } => {
format!("{typ} {constant}")
Expand All @@ -71,9 +72,7 @@ fn value(function: &Function, id: ValueId) -> String {
let elements = vecmap(array, |element| value(function, *element));
format!("[{}]", elements.join(", "))
}
Value::Param { .. } | Value::Instruction { .. } => {
function.dfg.resolve_replaced_value_id(id).to_string()
}
Value::Param { .. } | Value::Instruction { .. } => id.to_string(),
}
}

Expand Down
11 changes: 6 additions & 5 deletions crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@ impl Context {
for instruction in instructions {
self.push_instruction(function, block, instruction);
}

let terminator =
function.dfg[block].unwrap_terminator().map_values(|value| self.get_value(value));
let terminator = function.dfg[block]
.unwrap_terminator()
.map_values(|value| self.get_value(function.dfg.resolve(value)));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are there any other terminator mappings we need to update as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

There will be in DIE, once that PR's merged in these changes.


function.dfg.set_block_terminator(block, terminator);
self.block_queue.extend(function.dfg[block].successors());
Expand All @@ -74,8 +74,9 @@ impl Context {
block: BasicBlockId,
id: InstructionId,
) {
let instruction = function.dfg[id].map_values(|id| self.get_value(id));
let results = function.dfg.instruction_results(id).to_vec();
let instruction =
function.dfg[id].map_values(|id| self.get_value(function.dfg.resolve(id)));
let results = vecmap(function.dfg.instruction_results(id), |id| function.dfg.resolve(*id));

let ctrl_typevars = instruction
.requires_ctrl_typevars()
Expand Down
3 changes: 2 additions & 1 deletion crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,8 @@ impl<'f> Context<'f> {
fn push_instruction(&mut self, id: InstructionId) {
let instruction = self.function.dfg[id].map_values(|id| self.translate_value(id));
let instruction = self.handle_instruction_side_effects(instruction);
let results = self.function.dfg.instruction_results(id).to_vec();
let results = self.function.dfg.instruction_results(id);
let results = vecmap(results, |id| self.function.dfg.resolve(*id));

let ctrl_typevars = instruction
.requires_ctrl_typevars()
Expand Down
5 changes: 3 additions & 2 deletions crates/noirc_evaluator/src/ssa_refactor/opt/inlining.rs
Original file line number Diff line number Diff line change
Expand Up @@ -362,13 +362,14 @@ impl<'function> PerFunctionContext<'function> {
fn push_instruction(&mut self, id: InstructionId) {
let instruction = self.source_function.dfg[id].map_values(|id| self.translate_value(id));
let results = self.source_function.dfg.instruction_results(id);
let results = vecmap(results, |id| self.source_function.dfg.resolve(*id));

let ctrl_typevars = instruction
.requires_ctrl_typevars()
.then(|| vecmap(results, |result| self.source_function.dfg.type_of_value(*result)));
.then(|| vecmap(&results, |result| self.source_function.dfg.type_of_value(*result)));

let new_results = self.context.builder.insert_instruction(instruction, ctrl_typevars);
Self::insert_new_instruction_results(&mut self.values, results, new_results);
Self::insert_new_instruction_results(&mut self.values, &results, new_results);
}

/// Modify the values HashMap to remember the mapping between an instruction result's previous
Expand Down
3 changes: 2 additions & 1 deletion crates/noirc_evaluator/src/ssa_refactor/opt/unrolling.rs
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,8 @@ impl<'f> LoopIteration<'f> {

fn push_instruction(&mut self, id: InstructionId) {
let instruction = self.function.dfg[id].map_values(|id| self.get_value(id));
let results = self.function.dfg.instruction_results(id).to_vec();
let results = self.function.dfg.instruction_results(id);
let results = vecmap(results, |id| self.function.dfg.resolve(*id));

let ctrl_typevars = instruction
.requires_ctrl_typevars()
Expand Down