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.

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

Jump to bottom

feat(mem2reg): Remove trivial stores #5865

Closed
wants to merge 27 commits into from
Closed

feat(mem2reg): Remove trivial stores #5865

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
e0ad0a9
remove trivial stores
vezenovm Aug 29, 2024
5bbaca8
fmt
vezenovm Aug 29, 2024
a66009a
don't include arrays in last store
vezenovm Aug 29, 2024
ce31cb0
switch to using get_known_value and set_known_value
vezenovm Aug 30, 2024
45db90b
add from_rc flag to store
vezenovm Aug 30, 2024
96b70bd
cargo fmt
vezenovm Aug 30, 2024
6a0f8c3
Merge branch 'master' into mv/simplify-immediate-stores
vezenovm Aug 30, 2024
a595349
Merge branch 'master' into mv/simplify-immediate-stores
vezenovm Aug 30, 2024
82bc2ea
don't call mem2reg and die again to fix uhashmap
vezenovm Aug 30, 2024
6ce03eb
cargo fmt
vezenovm Aug 30, 2024
5c41302
comment out extra mem2reg and die
vezenovm Aug 30, 2024
568c083
remove last_loads approach and add some comments
vezenovm Aug 30, 2024
c1c2b61
cleanup
vezenovm Aug 30, 2024
6222963
Merge branch 'master' into mv/simplify-immediate-stores
vezenovm Aug 30, 2024
6ead23d
Merge branch 'master' into mv/simplify-immediate-stores
vezenovm Sep 3, 2024
dae5a44
cleanup
vezenovm Sep 3, 2024
e920768
Merge remote-tracking branch 'origin/mv/simplify-immediate-stores' in…
vezenovm Sep 3, 2024
4962db1
reduce diff
vezenovm Sep 3, 2024
31224eb
missed push
vezenovm Sep 3, 2024
adacab4
cleanup
vezenovm Sep 3, 2024
ea39ead
Merge branch 'master' into mv/simplify-immediate-stores
vezenovm Sep 3, 2024
20d20af
remove from_rc from Store and track in mem2reg
vezenovm Sep 3, 2024
2d8f5e6
remove unnecessary
vezenovm Sep 3, 2024
90a4fbe
move rc reload state tracking
vezenovm Sep 3, 2024
90d5715
improve handling of rc reload state and handle call when tracking
vezenovm Sep 4, 2024
96ea083
Merge branch 'master' into mv/simplify-immediate-stores
vezenovm Sep 4, 2024
0db5610
master merge
vezenovm Sep 4, 2024
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
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ impl<'block> BrilligBlock<'block> {
}
}
}
Instruction::Store { address, value } => {
Instruction::Store { address, value, .. } => {
let address_var = self.convert_ssa_single_addr_value(*address, dfg);
let source_variable = self.convert_ssa_value(*value, dfg);

Expand Down
8 changes: 6 additions & 2 deletions compiler/noirc_evaluator/src/ssa/function_builder/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,11 @@ impl FunctionBuilder {
/// at the given address. Expects that the address points somewhere
/// within a previous Allocate instruction.
pub(crate) fn insert_store(&mut self, address: ValueId, value: ValueId) {
self.insert_instruction(Instruction::Store { address, value }, None);
self.insert_instruction(Instruction::Store { address, value, from_rc: false }, None);
}

pub(crate) fn insert_store_from_rc(&mut self, address: ValueId, value: ValueId) {
self.insert_instruction(Instruction::Store { address, value, from_rc: true }, None);
}

/// Insert a binary instruction at the end of the current block.
Expand Down Expand Up @@ -478,7 +482,7 @@ impl FunctionBuilder {
let address = *address;
let new_load = self.insert_load(address, typ);
update_rc(self, new_load);
self.insert_store(address, new_load);
self.insert_store_from_rc(address, new_load);
}
}
}
Expand Down
8 changes: 4 additions & 4 deletions compiler/noirc_evaluator/src/ssa/ir/instruction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ pub(crate) enum Instruction {
Load { address: ValueId },

/// Writes a value to memory.
Store { address: ValueId, value: ValueId },
Store { address: ValueId, value: ValueId, from_rc: bool },

/// Provides a context for all instructions that follow up until the next
/// `EnableSideEffectsIf` is encountered, for stating a condition that determines whether
Expand Down Expand Up @@ -482,8 +482,8 @@ impl Instruction {
},
Instruction::Allocate => Instruction::Allocate,
Instruction::Load { address } => Instruction::Load { address: f(*address) },
Instruction::Store { address, value } => {
Instruction::Store { address: f(*address), value: f(*value) }
Instruction::Store { address, value, from_rc } => {
Instruction::Store { address: f(*address), value: f(*value), from_rc: *from_rc }
}
Instruction::EnableSideEffectsIf { condition } => {
Instruction::EnableSideEffectsIf { condition: f(*condition) }
Expand Down Expand Up @@ -546,7 +546,7 @@ impl Instruction {
}
}

Instruction::Store { address, value } => {
Instruction::Store { address, value, .. } => {
f(*address);
f(*value);
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/noirc_evaluator/src/ssa/ir/printer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,8 @@ fn display_instruction_inner(
}
Instruction::Allocate => writeln!(f, "allocate"),
Instruction::Load { address } => writeln!(f, "load {}", show(*address)),
Instruction::Store { address, value } => {
writeln!(f, "store {} at {}", show(*value), show(*address))
Instruction::Store { address, value, from_rc } => {
writeln!(f, "store {} at {}, from_rc {}", show(*value), show(*address), *from_rc)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a big fan of including from_rc on every store instruction for a mem2reg-specific issue.

Can we add tracking to mem2reg specifically instead? E.g. if we see a load -> dec-rc -> store we mark that we can't remove the store?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look at switching to that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched to tracking the current rc reload per instruction with an Option<(ValueId, bool)>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah something looks to be failing. I had it passing before but I guess I made a bad change while cleaning up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had moved when I was calling my method to track the rc reload state, but this led to inadvertently calling the method on the wrong instruction id. This is now fixed and the PR is ready for review again.

Copy link
Contributor Author

@vezenovm vezenovm Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Looks to still be failing on the debugger actually as the debugger inserts foreign calls which are breaking up the expected block of this form:

    v72 = load v57
    inc_rc v72
    v73 = load v57
    inc_rc v73
    store v73 at v57
    store v72 at v60

In the debugger we see the following before mem2reg after the inlining pass:

    v76 = load v14
    inc_rc v76
    v77 = load v14
    inc_rc v77
    store v77 at v14
    call v80(Field 1, v76)
    inc_rc v76
    v81 = load v14
    inc_rc v81
    store v81 at v14
    store v76 at v26

}
Instruction::EnableSideEffectsIf { condition } => {
writeln!(f, "enable_side_effects {}", show(*condition))
Expand Down
8 changes: 4 additions & 4 deletions compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,7 @@ impl<'f> Context<'f> {
let value = new_values[address];
let address = *address;
self.insert_instruction_with_typevars(
Instruction::Store { address, value },
Instruction::Store { address, value, from_rc: false },
None,
call_stack.clone(),
);
Expand Down Expand Up @@ -740,9 +740,9 @@ impl<'f> Context<'f> {

Instruction::Constrain(lhs, rhs, message)
}
Instruction::Store { address, value } => {
Instruction::Store { address, value, from_rc } => {
self.remember_store(address, value, call_stack);
Instruction::Store { address, value }
Instruction::Store { address, value, from_rc }
}
Instruction::RangeCheck { value, max_bit_size, assert_message } => {
// Replace value with `value * predicate` to zero out value when predicate is inactive.
Expand Down Expand Up @@ -869,7 +869,7 @@ impl<'f> Context<'f> {
for (address, store) in store_values {
let address = *address;
let value = store.old_value;
let instruction = Instruction::Store { address, value };
let instruction = Instruction::Store { address, value, from_rc: false };
// Considering the location of undoing a store to be the same as the original store.
self.insert_instruction_with_typevars(instruction, None, store.call_stack.clone());
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ impl<'a> SliceCapacityTracker<'a> {
}
}
}
Instruction::Store { address, value } => {
Instruction::Store { address, value, .. } => {
let value_typ = self.dfg.type_of_value(*value);
if value_typ.contains_slice_element() {
self.compute_slice_capacity(*value, slice_sizes);
Expand Down
13 changes: 11 additions & 2 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 @@ -253,16 +253,19 @@ impl<'f> PerFunctionContext<'f> {

// If the load is known, replace it with the known value and remove the load
if let Some(value) = references.get_known_value(address) {
let result = self.inserter.function.dfg.instruction_results(instruction)[0];
self.inserter.map_value(result, value);
self.instructions_to_remove.insert(instruction);
} else {
references.mark_value_used(address, self.inserter.function);

references.expressions.insert(result, Expression::Other(result));
references.aliases.insert(Expression::Other(result), AliasSet::known(result));
references.set_known_value(result, address);

self.last_loads.insert(address, instruction);
}
}
Instruction::Store { address, value } => {
Instruction::Store { address, value, from_rc } => {
let address = self.inserter.function.dfg.resolve(*address);
let value = self.inserter.function.dfg.resolve(*value);

Expand All @@ -274,6 +277,12 @@ impl<'f> PerFunctionContext<'f> {
self.instructions_to_remove.insert(*last_store);
}

if let Some(known_value) = references.get_known_value(value) {
if known_value == address && !from_rc {
self.instructions_to_remove.insert(instruction);
}
}

references.set_known_value(address, value);
references.last_stores.insert(address, instruction);
}
Expand Down
4 changes: 3 additions & 1 deletion test_programs/execution_success/brillig_loop_size_regression/src/main.nr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ struct EnumEmulation {
unconstrained fn main() -> pub Field {
let mut emulated_enum = EnumEmulation { a: Option::some(1), b: Option::none(), c: Option::none() };

// Do a copy to optimize out loads in the loop
let copy_enum = emulated_enum;
for _ in 0..1 {
assert_eq(emulated_enum.a.unwrap(), 1);
assert_eq(copy_enum.a.unwrap(), 1);
}

emulated_enum.a = Option::some(2);
Expand Down
Loading