feat: merge basic blocks algorithm

[acl-cqc]

Any basic block with a single successor that has no other predecessors can be merged. (So long as the successor is not the exit block; nor the entry block, as that implicitly has another predecessor).

Implement via Replace (2 BBs - > one BB containing 2 DFGs, which adopt the original BB's children) and 2*InlineDFG.

Closes #561.

[acl-cqc]

I feel this shape is a reasonable test, so am not tooo worried about adding more control-flow structures. However it'd definitely be good to add more differently-typed blocks e.g. with different numbers of inputs/outputs, values in the branch-control predicate or not, etc.

[Cobord]

Is there anything that would want prevention of making basic blocks too big? I had this where if the merged would be too big (option input for what that limit is) for a pass to do quickly enough (because only had smaller basic blocks in mind when doing that one) it would just leave them as they were.

[acl-cqc]

Is there anything that would want prevention of making basic blocks too big? I had this where if the merged would be too big (option input for what that limit is) for a pass to do quickly enough (because only had smaller basic blocks in mind when doing that one) it would just leave them as they were.

Hi @Cobord, thanks for the interest! :)

Hmmm, well, that's quite a complex question. So in general, we favour bringing more code together into fewer basic blocks as this will make it easier to find more opportunities for other optimisations. If a particular pass is inefficient (supralinear or quadratic in BB-size, say) then that could be an issue. However, firstly note that the existing basic block boundaries are not necessarily the best boundaries - blocks are dataflow graph with parallelism, so we could break the block on any cut across. So, it'd be better for optimizations to pick their own arbitrary divisions. The argument for not merging, thus, would be if we wanted to keep the divisions between BBs that existed, presumably because we thought those were good divisions - suggesting that we've already done some deliberate partitioning; and we would only want to do such deliberate partitioning ahead of the pass needing it, if there were multiple passes (or repeat instances of the same pass) that want this partitioning.

I think we are a long way from that being a problem!

A second reason for merging basic blocks is that it'll simplify the step of detecting structured control flow (conditionals and tail loops) i.e. these passes will not need to deal with single-pred-single-succ chains. Hence, the solution I'd prefer (if we do want to avoid blocks getting too big) would be to skip the InlineDFGs step, rather than leave BBs unmerged. One could pass in a filter-function should_inline_DFGs: impl Fn<BasicBlockID> -> bool, say, that defaults to always-true. I think that's something we could add (straightforwardly) in a follow-up PR if needed, though.

[doug-q]

I think we should put this into a new hugr-passes crate. This doesn't need any private access into hugr; putting passes into another crate ensures it stays clean and ensures we don't do anything naughty.

[codecov]

Codecov Report

Attention: Patch coverage is 91.84549% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 85.90%. Comparing base (529f553) to head (7819211).
Report is 24 commits behind head on main.

❗ Current head 7819211 differs from pull request most recent head 6e4a3fe. Consider uploading reports for the commit 6e4a3fe to get more accurate results

Files	Patch %	Lines
hugr/src/algorithm/merge_bbs.rs	91.84%	2 Missing and 17 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #956      +/-   ##
==========================================
+ Coverage   85.88%   85.90%   +0.01%     
==========================================
  Files          78       80       +2     
  Lines       14407    14765     +358     
  Branches    14407    14765     +358     
==========================================
+ Hits        12374    12684     +310     
- Misses       1397     1418      +21     
- Partials      636      663      +27     

Flag	Coverage Δ
rust	85.90% <91.84%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[acl-cqc]

I think we should put this into a new hugr-passes crate. This doesn't need any private access into hugr; putting passes into another crate ensures it stays clean and ensures we don't do anything naughty.

I am generally for this, but it's not quite as straightforward as it sounds - for starters, we should take as much of the algorithm module as we can into hugr_passes as well, including nest_cfgs in particular (we might not want to move const_fold as the standard extensions in the main Hugr crate needs to depend upon that and we can't have cyclic dependencies). However the test code for nest_cfgs is also used by the OutlineCfg rewrite and that's not easily separated (and its a lot to duplicate). Secondly we would need to make trait HugrMut pub, and while moving this stuff out is exactly the kind of reason why we should make HugrMut pub, this is definitely something to do in a separate refactoring PR. I'm having a go at this now, but I think there is enough "meat" in this merge-bbs PR to start looking at it
as it stands (regardless of the order in which merging and moving happens).....

[cqc-alec]

I think we should put this into a new hugr-passes crate. This doesn't need any private access into hugr; putting passes into another crate ensures it stays clean and ensures we don't do anything naughty.

I am generally for this, but it's not quite as straightforward as it sounds - for starters, we should take as much of the algorithm module as we can into hugr_passes as well, including nest_cfgs in particular (we might not want to move const_fold as the standard extensions in the main Hugr crate needs to depend upon that and we can't have cyclic dependencies). However the test code for nest_cfgs is also used by the OutlineCfg rewrite and that's not easily separated (and its a lot to duplicate). Secondly we would need to make trait HugrMut pub, and while moving this stuff out is exactly the kind of reason why we should make HugrMut pub, this is definitely something to do in a separate refactoring PR. I'm having a go at this now, but I think there is enough "meat" in this merge-bbs PR to start looking at it as it stands (regardless of the order in which merging and moving happens).....

The main issue seems to be that a lot of test code for non-pass functionality depends on passes that are currently implemented in HUGR. While this is convenient, we could probably organize things differently. Some of that test code (including the tests that use constant_fold_pass) could just as well live in the hugr-passes crate.

But I agree that this needn't wait for that refactor.

[doug-q]

The main issue seems to be that a lot of test code for non-pass functionality depends on passes that are currently implemented in HUGR. While this is convenient, we could probably organize things differently. Some of that test code (including the tests that use constant_fold_pass) could just as well live in the hugr-passes crate.

But I agree that this needn't wait for that refactor.

My understanding is that we could put the passes into hugr-crate, add hugr-crate as a dev-dependency to hugr and it would work. i.e. circular dependencies are allowed in that case.

[cqc-alec]

Could just check the size here.

[cqc-alec]

Looks like a debug line left in by mistake?

[acl-cqc]

Oops, thanks!