🐛 Multi-output nodes

[simon1hofmann]

Description

Fixes a corner case in ortho where a gate has multiple output nodes connected to it.
Thanks to @hibenj for pointing out this solution in his implementation of orthogonal_ordering_network.

Checklist:

• The pull request only contains commits that are related to it.
• I have added appropriate tests and documentation.
• I have added a changelog entry.
• I have created/adjusted the Python bindings for any new or updated functionality.
• I have made sure that all CI jobs on GitHub pass.
• The pull request introduces no new warnings and follows the project's style guidelines.

[hibenj]

The code seems to be working. Before merging you might test the following: I guess you already checked for all benchmarks, that this is working. But you could also check, if the number of added rows corresponds to the number of multi PO nodes. But since you first determine the layout size and then insert the PO nodes, everything should be working perfectly. I just know i had issues with this before.

[simon1hofmann]

The code seems to be working. Before merging you might test the following: I guess you already checked for all benchmarks, that this is working. But you could also check, if the number of added rows corresponds to the number of multi PO nodes. But since you first determine the layout size and then insert the PO nodes, everything should be working perfectly. I just know i had issues with this before.

Additional rows are added based on the number of multi-output nodes in determine_layout_size():

// for multi-output nodes, add another row
y += num_multi_output_pos;

[hibenj]

The code seems to be working. But since you already iterate through all pos at the beginning of orthogonal_impl and therefore compute all multi_out_nodes, you might be able to use them in the main loop of ortho to place the multi_out_nodes. With this implementation you need to iterate through all pos twice.
So i approved it, since its working, but i guess its up to @marcelwa if this implementation makes sense.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.22%. Comparing base (b07825b) to head (b287f35).
Report is 64 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #451   +/-   ##
=======================================
  Coverage   98.22%   98.22%           
=======================================
  Files         214      214           
  Lines       28883    28907   +24     
  Branches     1368     1368           
=======================================
+ Hits        28369    28394   +25     
+ Misses        514      513    -1     

Files with missing lines	Coverage Δ
.../fiction/algorithms/physical_design/orthogonal.hpp	96.36% <100.00%> (+1.26%)	⬆️
test/algorithms/physical_design/orthogonal.cpp	100.00% <100.00%> (ø)
test/utils/blueprints/network_blueprints.hpp	100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b07825b...b287f35. Read the comment docs.

[simon1hofmann]

The code seems to be working. But since you already iterate through all pos at the beginning of orthogonal_impl and therefore compute all multi_out_nodes, you might be able to use them in the main loop of ortho to place the multi_out_nodes. With this implementation you need to iterate through all pos twice. So i approved it, since its working, but i guess its up to @marcelwa if this implementation makes sense.

The problem ist that multi-output POs usually appear in a different order in foreach_po compared to foreach_node.
I now added a check for the appearance of multi-output POs in the network and only if they are present, the POs are placed after the main loop. Otherwise, ortho places all POs in the main loop.

[simon1hofmann]

The code seems to be working. But since you already iterate through all pos at the beginning of orthogonal_impl and therefore compute all multi_out_nodes, you might be able to use them in the main loop of ortho to place the multi_out_nodes. With this implementation you need to iterate through all pos twice. So i approved it, since its working, but i guess its up to @marcelwa if this implementation makes sense.

The problem ist that multi-output POs usually appear in a different order in foreach_po compared to foreach_node. I now added a check for the appearance of multi-output POs in the network and only if they are present, the POs are placed after the main loop. Otherwise, ortho places all POs in the main loop.

Actually, this can also happen for networks without mulit-ouput POs, which explains why the layout for clpl was not equivalent. I added another check to test that.

[simon1hofmann]

@marcelwa I refactored the code to always place POs at the end, as this will solve both problems simultaneously.

[marcelwa]

Just two tiny comments, then it is ready to be merged

[marcelwa]

Many thanks!