Skip to content

Commit

Permalink
chore: pull out SSA changes from sync PR (#7209)
Browse files Browse the repository at this point in the history
This PR pulls out some more useful changes from the sync PR notably a
bug fix which is blocking @nventuro
  • Loading branch information
TomAFrench authored Jun 27, 2024
1 parent 7ed7558 commit 141e137
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 31 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,12 @@ pub(crate) struct FunctionInserter<'f> {
pub(crate) function: &'f mut Function,

values: HashMap<ValueId, ValueId>,
const_arrays: HashMap<im::Vector<ValueId>, ValueId>,
}

impl<'f> FunctionInserter<'f> {
pub(crate) fn new(function: &'f mut Function) -> FunctionInserter<'f> {
Self { function, values: HashMap::default() }
Self { function, values: HashMap::default(), const_arrays: HashMap::default() }
}

/// Resolves a ValueId to its new, updated value.
Expand All @@ -34,10 +35,17 @@ impl<'f> FunctionInserter<'f> {
super::value::Value::Array { array, typ } => {
let array = array.clone();
let typ = typ.clone();
let new_array = array.iter().map(|id| self.resolve(*id)).collect();
let new_id = self.function.dfg.make_array(new_array, typ);
self.values.insert(value, new_id);
new_id
let new_array: im::Vector<ValueId> =
array.iter().map(|id| self.resolve(*id)).collect();
if self.const_arrays.get(&new_array) == Some(&value) {
value
} else {
let new_array_clone = new_array.clone();
let new_id = self.function.dfg.make_array(new_array, typ);
self.values.insert(value, new_id);
self.const_arrays.insert(new_array_clone, new_id);
new_id
}
}
_ => value,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,6 @@ impl<'f> Context<'f> {
let old_condition = *condition;
let then_condition = self.inserter.resolve(old_condition);

let one = FieldElement::one();
let old_stores = std::mem::take(&mut self.store_values);
let old_allocations = std::mem::take(&mut self.local_allocations);
let branch = ConditionalBranch {
Expand All @@ -393,15 +392,6 @@ impl<'f> Context<'f> {
};
self.condition_stack.push(cond_context);
self.insert_current_side_effects_enabled();
// Optimization: within the then branch we know the condition to be true, so replace
// any references of it within this branch with true. Likewise, do the same with false
// with the else branch. We must be careful not to replace the condition if it is a
// known constant, otherwise we can end up setting 1 = 0 or vice-versa.
if self.inserter.function.dfg.get_numeric_constant(old_condition).is_none() {
let known_value = self.inserter.function.dfg.make_constant(one, Type::bool());

self.inserter.map_value(old_condition, known_value);
}
vec![self.branch_ends[if_entry], *else_destination, *then_destination]
}

Expand All @@ -414,7 +404,6 @@ impl<'f> Context<'f> {
self.insert_instruction(Instruction::Not(cond_context.condition), CallStack::new());
let else_condition = self.link_condition(else_condition);

let zero = FieldElement::zero();
// Make sure the else branch sees the previous values of each store
// rather than any values created in the 'then' branch.
let old_stores = std::mem::take(&mut cond_context.then_branch.store_values);
Expand All @@ -429,21 +418,12 @@ impl<'f> Context<'f> {
local_allocations: old_allocations,
last_block: *block,
};
let old_condition = else_branch.old_condition;
cond_context.then_branch.local_allocations.clear();
cond_context.else_branch = Some(else_branch);
self.condition_stack.push(cond_context);

self.insert_current_side_effects_enabled();
// Optimization: within the then branch we know the condition to be true, so replace
// any references of it within this branch with true. Likewise, do the same with false
// with the else branch. We must be careful not to replace the condition if it is a
// known constant, otherwise we can end up setting 1 = 0 or vice-versa.
if self.inserter.function.dfg.get_numeric_constant(old_condition).is_none() {
let known_value = self.inserter.function.dfg.make_constant(zero, Type::bool());

self.inserter.map_value(old_condition, known_value);
}

assert_eq!(self.cfg.successors(*block).len(), 1);
vec![self.cfg.successors(*block).next().unwrap()]
}
Expand Down Expand Up @@ -471,11 +451,6 @@ impl<'f> Context<'f> {
// known to be true/false within the then/else branch respectively.
self.insert_current_side_effects_enabled();

// We must map back to `then_condition` here. Mapping `old_condition` to itself would
// lose any previous mappings.
self.inserter
.map_value(cond_context.then_branch.old_condition, cond_context.then_branch.condition);

// While there is a condition on the stack we don't compile outside the condition
// until it is popped. This ensures we inline the full then and else branches
// before continuing from the end of the conditional here where they can be merged properly.
Expand Down

0 comments on commit 141e137

Please sign in to comment.