chore(ssa): Refactor unrolling

[aakoshh]

Description

Problem*

Some refactoring opportunities I saw to make the code easier to audit.

Summary*

A few changes to unrolling that shouldn't alter its behaviour, but hopefully make the code cleaner:

• Remove the elimination of inc_rc and dec_rc from ACIR functions: these have nothing to do with unrolling (we only count instructions for Brillig to see if a loop is small), and when we insert instructions into the block we unroll into, the simplification inevitably calls is_handled_by_runtime, which returns false for these, thus they will not be inserted anyway.
• Switch to std::collections::HashSet from im::HashSet as we don't need deterministic data structures for unrolling, and hopefully O(1) is faster than O(log(n)).
• Get rid of unroll_loop_block_helper: it was only called from unroll_loop_block and didn't seem to increase clarity.
• Get rid of the recursion in unroll_each: previously when Loops::unroll_each encountered a nested loop, it stopped consuming yet_to_unroll, created a new Loops, made a recursive call to itself, then merged the results. We would probably not have loops nested deep enough to cause a stack overflow, however:
  • I found it a difficult to grok; now we have a simpler loop construct with a succession of continue conditions.
  • It never processed the rest of yet_to_unroll, so all the unrelated "sibling" loops we discovered in find_all were wasted. Now we finish going through them before refreshing the context.
  • It required carefully passing the failed_to_unroll to the next Loops to avoid redoing them again.
• Move the failed_to_unroll and modified_blocks fields from Loops into the unroll_each method: Loops are used from multiple modules now (e.g. LICM), and these were only relevant to unrolling. As mentioned they also required passing from one context to the next. By keeping them local there is less reasoning to be done about their scope.
• Removed Loops::unroll_each and moved all functionality into Function::try_unroll_loops. The latter only forwarded to the former, and now that it's no longer recursive, I think it's cleaner to keep all unrolling in there, rather than Loops, because during the iteration we do call Loops::find_all to refresh the context, which makes it look a bit odd to call it on self.
• Only iterating reachable blocks.
• Added some extra assertions where empty values were returned.

Additional Context

Documentation*

Check one:

• No documentation needed.
• Documentation included in this PR.
• [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

• I have tested the changes locally.
• I have formatted the changes with Prettier and/or cargo fmt on default settings.

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: b3e4166	Previous: d94400f	Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob	238 s	177 s	1.34
test_report_zkpassport_noir-ecdsa_	3 s	2 s	1.50

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[TomAFrench]

Looks sensible to me.