fix(ssa refactor): mem2reg handles muts only #1654
Conversation
There was a problem hiding this comment.
I like the simplifications from removing array address/offset handling but is there any way we can keep allowing references to be passed to functions or returned by functions? I'd like this feature for when references are eventually added to Noir.
It was also mentioned during standup that this PR fixes some bugs which allows some examples to now compile. Which bugs/examples are these?
| let blocks = function.reachable_blocks(); | ||
| let is_single_block = blocks.len() == 1; | ||
| for block in function.reachable_blocks() { |
There was a problem hiding this comment.
The second call to reachable_blocks can be replaced with blocks
| if let Some(address) = self.try_const_address(*value, dfg) { | ||
| self.alloc_ids_used_externally.insert(address.alloc_id()); | ||
| } | ||
| assert!(!self.last_stores.contains_key(value), "Mutable vars are loaded before being returned - if this pattern changes, so do our safety assumptions."); |
There was a problem hiding this comment.
I'd like to retain the ability to pass in or return mutable references to functions for implementing references later on. Does handling this cause any bugs that were present previously?
|
After testing #1675, I see that some tests were failing before this PR because stores weren't being removed, which may reference an allocate instruction, which leads to DIE not removing the allocate instruction and acir-gen panicking as a result. |
|
Obsoleted by #1677 |
Description
Problem*
Many of the assumptions upon which
mem2regwas designed have changed due to the refactoring of array handling:Before: An allocation instruction represents the address of an array
Now: No longer true - allocations are now only used to represent a mutable value. Whether or not the value is an array is inconsequential.
Before: Allocation addresses could be passed between functions via calling or returning.
Now: No longer true - an allocation is now always scoped to the blocks of a single function.
Summary*
Thanks to the above, performing this pass is now far simpler. We can now infer that if a function contains only a single block, it is safe to remove store instructions, because their effects are limited to the block in question.
Asserts have been added to make sure that the assumption that alloc addresses are not passed between functions does not regress.
Also, we are no longer concerned with arrays or their indexing logic - all of which is now removed. Such optimisations are now covered elsewhere by the new array model in conjunction with instruction simplification.
Documentation
This PR requires documentation updates when merged.
Additional Context
PR Checklist*
cargo fmton default settings.