feat: LocalizeEdges pass #2237

acl-cqc · 2025-05-20T08:07:29Z

closes #1234

Based off @doug-q's #1912 but with "outside-in" transformation step avoiding much data storage (e.g. ParentSourceMap).

…-local

…non-local

codecov · 2025-05-20T08:10:00Z

Codecov Report

Attention: Patch coverage is 96.33028% with 20 lines in your changes missing coverage. Please review.

Project coverage is 82.17%. Comparing base (9985143) to head (b4417f3).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
hugr-passes/src/non_local/localize.rs	96.11%	11 Missing ⚠️
hugr-passes/src/non_local.rs	96.56%	6 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2237      +/-   ##
==========================================
+ Coverage   82.00%   82.17%   +0.17%     
==========================================
  Files         234      238       +4     
  Lines       41618    43305    +1687     
  Branches    37532    39217    +1685     
==========================================
+ Hits        34127    35587    +1460     
- Misses       5518     5722     +204     
- Partials     1973     1996      +23

Flag	Coverage Δ
python	`85.34% <ø> (+0.12%)`	⬆️
rust	`81.84% <96.33%> (+0.19%)`	⬆️

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.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…used)

clippy objected to use of `mut` inside a debug_assert!, even though the mutated thing was local to the assert...

…dparent

aborgna-q

Adding some comments, I still need to finish localize.rs

aborgna-q · 2025-05-21T14:15:27Z

hugr-passes/src/non_local.rs

+/// [ComposablePass] that converts all non-local edges in a Hugr
+/// into local ones, by inserting extra inputs to container nodes
+/// and extra outports to Input nodes.
+pub struct LocalizeEdges;


Suggested change

pub struct LocalizeEdges;

#[derive(Clone, Debug, Hash)]

pub struct LocalizeEdges;

aborgna-q · 2025-05-21T14:18:18Z

hugr-passes/src/non_local.rs

+
+#[derive(derive_more::Error, derive_more::Display, derive_more::From, Debug, PartialEq)]
+#[non_exhaustive]
+pub enum LocalizeEdgesError {}


We really need to start checking for rustdocs...

hugr/hugr-passes/src/lib.rs

Line 2 in 2d97665

#![expect(missing_docs)] // TODO: Fix...

aborgna-q · 2025-05-21T14:19:30Z

hugr-passes/src/non_local.rs

+    fn run(&self, hugr: &mut H) -> Result<Self::Result, Self::Error> {
+        remove_nonlocal_edges(hugr)
+    }
+}

 /// Returns an iterator over all non local edges in a Hugr.


drive-by: This is no longer "all nl edges", but the ones that interact with the entrypoint descendants.

Right. Or rather, it's ones that are incoming to the subtree. So actually only one endpoint need be in the subtree.

So my thinking here was that there were two reasonable policies:

All nonlocal edges within the Hugr, ignoring hugr.entrypoint().

All nonlocal edges entirely beneath the entrypoint.

I meant to go for the latter, on the grounds that you can then get the former behaviour by passing entrypoint==root. Clearly I missed the latter ;-), I think the sensible thing is to put in that check.

However, if you think it'd be better to do this in stages (e.g. have two methods _root and _entrypoint, make the unsuffixed version delegate to _root but @deprecate it, and maybe eventually rename the _entrypoint variant back to no-suffix) then happy to do that

Agreed that "All nonlocal edges entirely beneath the entrypoint" is the way to go.
I think most passes should avoid out-of-tree modifications unless explicitly stated (e.g. defining a function in the module).

This should be compatible with the pre-entrypoint API.

aborgna-q · 2025-05-21T14:21:28Z

hugr-passes/src/non_local.rs

-            hugr.linked_outputs(node, in_p)
-                .any(|(neighbour_node, _)| parent != hugr.get_parent(neighbour_node))
-                .then_some((node, in_p))
+            let (src, _) = hugr.single_linked_output(node, in_p)?;


We are also now missing edges that connect out of the entrypoint descendants. (I realize it's annoying to check and avoid duplicates :/)

Argh. Yes.

So my thinking was that for transformation, we should mutate the Hugr only beneath the entrypoint, in which case we only want edges with both ends beneath the entrypoint.

Counterexamples?

An edge entering the subtree from a sibling of the entrypoint. This requires retargetting that edge to the entrypoint, and mutating the entrypoint node. Since no node outside the subtree is changed, I guess you could argue this is allowed, although I don't think I would myself....(looks to me like you've changed an edge outside the subtree!)

Unless the entrypoint is a DataflowBlock, the only possible NL edge leaving it would be an Ext edge from the entrypoint itself (to a descendant of a sibling). I think that edge is outside the subtree....(indeed, localizing it does not even mutate the entrypoint node)

If the entrypoint is a Dataflow Block, then Dom edges could leave its children to (descendants of) other DFB siblings-of-the-entrypoint, i.e. outside the tree. But clearly localizing those is gonna involve widespread changes outside the subtree (as well as to the entrypoint node itself).

aborgna-q · 2025-05-21T14:37:42Z

hugr-passes/src/non_local.rs

+    // Group all the non-local edges in the graph by target node,
+    // storing for each the source and type (well-defined as these are Value edges).
+    let nonlocal_edges_map: HashMap<_, _> = nonlocal_edges(hugr)
+        .filter_map(|(node, inport)| {


Why is this a filter_map instead of a map?

Good spot, we've already skipped over inports without a single_linked_output in nonlocal_edges(hugr). Thanks!

aborgna-q · 2025-05-21T14:43:23Z

hugr-passes/src/non_local.rs

+            else {
+                panic!("impossible")
+            };
+            Some((node, (source, ty)))


Can't we have multiple nl edges going to the same node?
This will drop all but one while collecting

Ooh good point. ("All compliments accrue to Doug but all shortcomings are my fault" 😉 😉 ). Yes, we dedup sources in the ExtraSourceReqs map (which is correct) but should not do so here. Thanks!

Will add a test.

aborgna-q · 2025-05-21T14:47:42Z

hugr-passes/src/non_local.rs

+    v.into_iter().map(|(_, t)| t.clone())
+}
+
+pub fn remove_nonlocal_edges<H: HugrMut>(hugr: &mut H) -> Result<(), LocalizeEdgesError> {


hugr-passes/src/non_local/localize.rs

doug-q and others added 30 commits February 6, 2025 14:32

wip

c328a18

add pass

cd2348d

oops

e04390e

conditional

903acc2

remove dbg

a87d705

refactor

11879a8

works

5f09ba9

wip

8f0b164

feat: Add Type::as_sum and SumType::variants.

af2959a

coverage

2e3e282

Merge remote-tracking branch 'origin/doug/sum-utils' into doug/un-non…

faeab31

…-local

wip

dc95bc9

wip

9f619b4

feat: Add HugrMutInternals::insert_ports

de0c513

fixes

f096dd7

with_prelude

504afe5

Merge remote-tracking branch 'origin/doug/insert-ports' into doug/un-…

983c69c

…non-local

works

6cc87a4

wip

461a5ab

Merge branch 'main' of gh:CQCL-DEV/hugr into doug/un-non-local

84633d9

get get_optype_mut

3a07aa3

fix merge

9d251df

tweak

9a8a5e2

fmt

dd4caa0

with_prelude

6dabc6b

wip

322facf

Merge branch 'main' of gh:CQCL-DEV/hugr into doug/un-non-local

558b5d5

comments, reorder analysis before transform, add tests of vec_insert

040f5db

simplify nonlocal_edges: inports can have at most one connected outport

bd68a4b

Remove NeedsSourcesMapIter/impl IntoIterator for &BBNeedsSourcesMap

ad1714c

acl-cqc added 5 commits May 19, 2025 19:01

Remove ParentSourceMap, WorkItem (undoing the TEMP)

c1b07b8

Add Types into ControlWorkItem

1518d4e

Update DFB inputs and sum_rows - tests now passing

e019f04

Add just_types helper

a41ae95

Renaming, add struct + impl ComposablePass

68f3020

acl-cqc added 12 commits May 20, 2025 09:41

Undo move

4cc2948

separate errors, deprecate NonLocalEdgesError

c4000e8

Convert ControlWorkItem into a simple function

8608a5f

Use optype_mut

aee31a1

Move BBNeedsSourcesMap(Builder) into submodule non_local/localize.rs

90e1029

Remove WorkItem.target as redundant (node is also key in map, port un…

6e27b62

…used)

Inline build_needs_sources_map, BBNeedsSourcesMapBuilder

fa10f7b

Tidy debug-asserts, don't use delegate

78ad38c

Don't import delegate

4681351

LocalizeEdgesError doesn't actually need any variants, hmmm

ac7a234

comments

e9c0f08

Rewrite assertion to make clippy happy

b069c1e

clippy objected to use of `mut` inside a debug_assert!, even though the mutated thing was local to the assert...

acl-cqc marked this pull request as ready for review May 20, 2025 13:56

acl-cqc requested a review from a team as a code owner May 20, 2025 13:56

acl-cqc requested a review from aborgna-q May 20, 2025 13:56

acl-cqc added 3 commits May 20, 2025 17:17

CFG test includes internal (dom) edge; extend debug assertion w/ gran…

2e4e85c

…dparent

Tidy checks

59f1c89

Rename BBNeedsSourceMap to ExtraSourceReqs, a few comments

e2f11d4

acl-cqc self-assigned this May 21, 2025

aborgna-q reviewed May 21, 2025

View reviewed changes

aborgna-q self-requested a review May 21, 2025 15:00

acl-cqc added 3 commits May 23, 2025 11:53

Remove unnecessary filter; HashMap -> Vec so no dedup

ceaa552

Docs

aa2daf8

Improve ExtraSourceReqs::get

b4417f3

	pub struct LocalizeEdges;
	#[derive(Clone, Debug, Hash)]
	pub struct LocalizeEdges;

feat: LocalizeEdges pass #2237

Are you sure you want to change the base?

feat: LocalizeEdges pass #2237

Conversation

acl-cqc commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aborgna-q left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acl-cqc May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

acl-cqc commented May 20, 2025 •

edited

Loading

codecov bot commented May 20, 2025 •

edited

Loading

acl-cqc May 23, 2025 •

edited

Loading