Skip to content

chore(mem2reg): removing seemingly unnecessary code #9616

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.

Sign up for GitHub

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

Closed
asterite wants to merge 4 commits into from
Closed

chore(mem2reg): removing seemingly unnecessary code #9616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f51d3fe
chore(mem2reg): remove some code that seems to be dead
asterite Aug 22, 2025
ec93cd3
Remove dead code in keep_last_store
asterite Aug 22, 2025
a874599
Remove seemingly uneeded for loop when handling return terminator
asterite Aug 22, 2025
0decd4d
No need to check func params as they are also block params
asterite Aug 22, 2025
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
53 changes: 2 additions & 51 deletions compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,10 +180,7 @@ impl<'f> PerFunctionContext<'f> {
}

let mut all_terminator_values = HashSet::default();
let mut per_func_block_params: HashSet<ValueId> = HashSet::default();
for (block_id, references) in self.blocks.iter_mut() {
let block_params = self.inserter.function.dfg.block_parameters(*block_id);
per_func_block_params.extend(block_params.iter());
let terminator = self.inserter.function.dfg[*block_id].unwrap_terminator();
terminator.for_each_value(|value| {
all_terminator_values.insert(value);
Expand All @@ -203,12 +200,8 @@ impl<'f> PerFunctionContext<'f> {
// This rule does not apply to reference parameters, which we must also check for before removing these stores.
for (_, block) in self.blocks.iter() {
for (store_address, store_instruction) in block.last_stores.iter() {
let store_alias_used = self.is_store_alias_used(
store_address,
block,
&all_terminator_values,
&per_func_block_params,
);
let store_alias_used =
self.is_store_alias_used(store_address, block, &all_terminator_values);

let is_dereference = block
.expressions
Expand All @@ -231,7 +224,6 @@ impl<'f> PerFunctionContext<'f> {
store_address: &ValueId,
block: &Block,
all_terminator_values: &HashSet<ValueId>,
per_func_block_params: &HashSet<ValueId>,
) -> bool {
let reference_parameters = self.reference_parameters();

Expand All @@ -241,10 +233,6 @@ impl<'f> PerFunctionContext<'f> {
return true;
}

if per_func_block_params.contains(&alias) {
return true;
}

// Is any alias of this address an input to some function call, or a return value?
if self.instruction_input_references.contains(&alias) {
return true;
Expand Down Expand Up @@ -495,50 +483,19 @@ impl<'f> PerFunctionContext<'f> {
references.aliases.insert(Expression::Other(result), AliasSet::known(result));
}
Instruction::ArrayGet { array, .. } => {
let result = self.inserter.function.dfg.instruction_results(instruction)[0];

let array = *array;
let array_typ = self.inserter.function.dfg.type_of_value(array);
if array_typ.contains_reference() {
self.instruction_input_references
.extend(references.get_aliases_for_value(array).iter());
references.mark_value_used(array, self.inserter.function);

let expression = Expression::ArrayElement(array);

if let Some(aliases) = references.aliases.get_mut(&expression) {
aliases.insert(result);
}
}
}
Instruction::ArraySet { array, value, .. } => {
references.mark_value_used(*array, self.inserter.function);
let element_type = self.inserter.function.dfg.type_of_value(*value);

if element_type.contains_reference() {
let result = self.inserter.function.dfg.instruction_results(instruction)[0];
let array = *array;

let expression = Expression::ArrayElement(array);

let mut aliases = if let Some(aliases) = references.aliases.get_mut(&expression)
{
aliases.clone()
} else if let Some((elements, _)) =
self.inserter.function.dfg.get_array_constant(array)
{
let aliases = references.collect_all_aliases(elements);
self.set_aliases(references, array, aliases.clone());
aliases
} else {
AliasSet::unknown()
Copy link
Copy Markdown
Contributor

@jfecher jfecher Aug 22, 2025

Choose a reason for hiding this comment

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

Removing this whole section for aliases of arrayset instructions scares me haha.
I suppose it's indicative we don't have tests for it at the very least - I'd hesitate to say it's completely unnecessary but it does have me doubting.

Copy link
Copy Markdown
Collaborator Author

@asterite asterite Aug 22, 2025

Choose a reason for hiding this comment

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

I agree. At least some part of this PR must not be right because Aztec-Packages failed: http://ci.aztec-labs.com/bf55284bb1e49ea4
Now I'm checking if it also fails in the currently open PR for the noir sync (it's failing for another reason, but maybe the failure is before this PR).

};

aliases.unify(&references.get_aliases_for_value(*value));

references.expressions.insert(result, expression);
references.aliases.insert(expression, aliases);

// Similar to how we remember that we used a value in a `Store` instruction,
// take note that it was used in the `ArraySet`. If this instruction is not
// going to be removed at the end, we shall keep the stores to this value as well.
Expand Down Expand Up @@ -698,12 +655,6 @@ impl<'f> PerFunctionContext<'f> {
}
}
TerminatorInstruction::Return { return_values, .. } => {
// We need to appropriately mark each alias of a reference as being used as a return terminator argument.
// This prevents us potentially removing a last store from a preceding block or is altered within another function.
for return_value in return_values {
self.instruction_input_references
.extend(references.get_aliases_for_value(*return_value).iter());
}
// Removing all `last_stores` for each returned reference is more important here
// than setting them all to unknown since no other block should
// have a block with a Return terminator as a predecessor anyway.
Expand Down
46 changes: 8 additions & 38 deletions compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
use std::borrow::Cow;

use crate::ssa::ir::{
function::Function,
instruction::{Instruction, InstructionId},
value::ValueId,
};
use crate::ssa::ir::{function::Function, instruction::InstructionId, value::ValueId};

use super::alias_set::AliasSet;

Expand Down Expand Up @@ -174,27 +170,13 @@ impl Block {
/// last stores anyway, we don't inherit them from predecessors, so if one
/// block stores to an address and a descendant block loads it, this mechanism
/// does not affect the candidacy of the last store in the predecessor block.
fn keep_last_stores_for(&mut self, address: ValueId, function: &Function) {
self.keep_last_store(address, function);
fn keep_last_stores_for(&mut self, address: ValueId) {
self.last_stores.remove(&address);

for alias in (*self.get_aliases_for_value(address)).clone().iter() {
self.keep_last_store(alias, function);
}
}

/// Forget the last store to an address, to remove it from the set of instructions
/// which are candidates for removal at the end. Also marks the values in the last
/// store as used, now that we know we want to keep them.
fn keep_last_store(&mut self, address: ValueId, function: &Function) {
if let Some(instruction) = self.last_stores.remove(&address) {
// Whenever we decide we want to keep a store instruction, we also need
// to go through its stored value and mark that used as well.
match &function.dfg[instruction] {
Instruction::Store { value, .. } => {
self.mark_value_used(*value, function);
}
other => {
unreachable!("last_store held an id of a non-store instruction: {other:?}")
if let Some(expr) = self.expressions.get(&address) {
if let Some(aliases) = self.aliases.get(expr) {
for alias in aliases.iter() {
self.last_stores.remove(&alias);
}
}
}
Expand All @@ -203,7 +185,7 @@ impl Block {
/// Mark a value (for example an address we loaded) as used by forgetting the last store instruction,
/// which removes it from the candidates for removal.
pub(super) fn mark_value_used(&mut self, value: ValueId, function: &Function) {
self.keep_last_stores_for(value, function);
self.keep_last_stores_for(value);

// We must do a recursive check for arrays since they're the only Values which may contain
// other ValueIds.
Expand All @@ -214,18 +196,6 @@ impl Block {
}
}

/// Collect all aliases used by the given value list
pub(super) fn collect_all_aliases(
&self,
values: impl IntoIterator<Item = ValueId>,
) -> AliasSet {
let mut aliases = AliasSet::known_empty();
for value in values {
aliases.unify(&self.get_aliases_for_value(value));
}
aliases
}

pub(super) fn get_aliases_for_value(&self, value: ValueId) -> Cow<AliasSet> {
if let Some(expression) = self.expressions.get(&value) {
if let Some(aliases) = self.aliases.get(expression) {
Expand Down
Loading