Skip to content

Commit

Permalink
chore: Remove RC tracking in mem2reg (#6019)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves <!-- Link to GitHub Issue -->

#5935 added logic to remove
trivial stores, however, it also had to track whether the alst
instruction was an `inc_rc` or `dec_rc` instruction. After
AztecProtocol/aztec-packages#8448 we can remove
that logic.

## Summary\*

Removes `inside_rc_reload` from mem2reg, its tracking method, and its
ussage.

Before AztecProtocol/aztec-packages#8448 we were
creating extra trivial stores. The majority of the code size benefit
comes from that PR avoiding the creation of those stores in the first
place. This PR is truly simply a clean-up that removes unnecessary
logic. I do not foresee large bytecode size increases from this removal
as the majority of the benefit came in the new array layout.

## Additional Context


## Documentation\*

Check one:
- [X] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [X] I have tested the changes locally.
- [X] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
vezenovm authored Sep 12, 2024
1 parent fc5bb02 commit 6231602
Showing 1 changed file with 1 addition and 18 deletions.
19 changes: 1 addition & 18 deletions compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,6 @@ struct PerFunctionContext<'f> {
/// Track a value's last load across all blocks.
/// If a value is not used in anymore loads we can remove the last store to that value.
last_loads: HashMap<ValueId, (InstructionId, BasicBlockId)>,

/// Track whether the last instruction is an inc_rc/dec_rc instruction.
/// If it is we should not remove any known store values.
inside_rc_reload: bool,
}

impl<'f> PerFunctionContext<'f> {
Expand All @@ -135,7 +131,6 @@ impl<'f> PerFunctionContext<'f> {
blocks: BTreeMap::new(),
instructions_to_remove: BTreeSet::new(),
last_loads: HashMap::default(),
inside_rc_reload: false,
}
}

Expand Down Expand Up @@ -324,7 +319,7 @@ impl<'f> PerFunctionContext<'f> {
let known_value = references.get_known_value(value);
if let Some(known_value) = known_value {
let known_value_is_address = known_value == address;
if known_value_is_address && !self.inside_rc_reload {
if known_value_is_address {
self.instructions_to_remove.insert(instruction);
}
}
Expand Down Expand Up @@ -383,18 +378,6 @@ impl<'f> PerFunctionContext<'f> {
Instruction::Call { arguments, .. } => self.mark_all_unknown(arguments, references),
_ => (),
}

self.track_rc_reload_state(instruction);
}

fn track_rc_reload_state(&mut self, instruction: InstructionId) {
match &self.inserter.function.dfg[instruction] {
// We just had an increment or decrement to an array's reference counter
Instruction::IncrementRc { .. } | Instruction::DecrementRc { .. } => {
self.inside_rc_reload = true;
}
_ => self.inside_rc_reload = false,
}
}

fn check_array_aliasing(&self, references: &mut Block, array: ValueId) {
Expand Down

0 comments on commit 6231602

Please sign in to comment.