feat(ssa): Remove all non side effectual instructions from unreachable blocks during DIE#9238
feat(ssa): Remove all non side effectual instructions from unreachable blocks during DIE#9238
Conversation
vezenovm
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 7e38d85 | Previous: c7ce62a | Ratio |
|---|---|---|---|
rollup-root |
0.005 s |
0.004 s |
1.25 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
| v0 = allocate -> &mut u1 | ||
| v2 = make_array [u1 0, v0] : [(u1, &mut u1); 1] | ||
| v4 = array_get v2, index u32 2 -> u1 |
There was a problem hiding this comment.
Shouldn't the current DIE pass (without the changes in this PR) already remove these as unused?
There was a problem hiding this comment.
Yes it would. I can update the test, but after #9232 none of these will be removed.
There was a problem hiding this comment.
I am going to branch off of #9232 as this is mostly relevant for that case, and I'm not sure if this is the best solution anymore.
| return false; | ||
| } | ||
|
|
||
| self.is_unused(instruction_id, function) |
There was a problem hiding this comment.
I was wondering if we can find a way to refactor this so that self.is_unused isn't called twice (self.is_unused(...) || self.can_remove_with_unreachable_terminator(...) where the latter calls the former again).
I must be missing something, but when do we return from this construct anything but what is_unsed would have returned already? All we return is false from the early returns, and then end up in the same case as what the || already checked.
There was a problem hiding this comment.
Yeah good point, I'll refactor this
|
I worked on this a little fast. After fixing the logic there are some other bugs I am getting with uninstantiated variables. Converting back to a draft for now. |
2 participants
Description
Problem*
This helps resolve a failure with
regression_8994in https://github.com/noir-lang/noir/actions/runs/16346960365/job/46184088603?pr=9232. I decided to pull it out as it should apply more generally.No issue but I noticed after #9008 that we can be significantly more aggressive about remove instructions with
Unreachableterminators.Summary*
If we have a block that terminates with
unreachable, we can remove all instructions in the block (not just those after the unreachable), if the instruciton:Additional Context
Documentation*
Check one:
PR Checklist*
cargo fmton default settings.