feat: BorrowSquashPass to elide redundant borrow/return ops #1159

acl-cqc · 2025-10-06T08:26:33Z

closes #1072, closes #1074

Bails out (locally) if any dynamic indexing.
Parametrizable by a trait that allows detecting borrows/returns....this is perhaps overkill / being too generic, but does make a nice abstraction layer, so have kept it.
Will need updating (and all the serialized test Hugrs redoing) followin refactor!: consistent inout order in borrow array hugr#2621

The main questions here are about unsafe_get / unsafe_put (CQCL/guppylang#1165), and to some extent new_all_borrowed. At the moment the PR is in an awkward midway state towards supporting these but not really:

If there is no unsafe-get/put, and if all new_all_borroweds are generated by guppy (i.e. we know that they are immediately followed by returns into every element), then we can fix assume_all_present to true and get more optimization. Clobber is then essentially pointless (guppy does not generate borrow/return around any op that Clobbers).
If we can't assume "all borrows/returns generated by guppy" (e.g. if there is unsafe-get/put), then that would potentially be converting panicking programs (new_all_borrowed, borrow, return) into non-panicking ones. Hence, we have to fix assume_all_present to false, which loses some optimization.
- The best solution here would be a dataflow analysis tracking borrowedness everywhere, see Better Borrow-squashing by dataflow analysis #1176.

TODO:

Needs some tests with dynamic indexing,
and either test Clobber or remove it.

Other ways of regaining safety in the presence of unsafe_put/get, that are not as good dataflow analysis but potentially less work:

Elide Borrow-Return only if immediately after Return. (So, Return-Borrow-Return, just does the initial Return, in the case that the second return puts back in exactly the value borrowed.)
- This would not be as good as in (1.) above, but better than initial (2.)
- Not as good as Dataflow analysis: that could elide Borrow-Return without a preceding Return if the array were known (by dataflow analysis) to have been initialized to all-elements-present.
- Code gets more complicated: the main match block would have remember+to handle cases of initial-Return whereas currently it only tracks the first op being a Borrow.
- Could we instead do Return-Borrow-Return -> just the final return (not requiring that the returned value is the borrowed value)? This would preserve that the program panics, but might change the order of panics (among ops whose order is forced by a linear array value being fed between them, heh). So a question of what guarantees do we give about visible semantics.
- It becomes useful to return Clobber for operations like get (copies element) and set (swaps external element in), 'coz we might now have return/borrow 'pairs' with set/get inbetween (and we can elide the pair if the index is definitely different). Indeed we might want two variants of "clobber" - giving the state of the element after the op (definitely borrowed, definitely not)

codecov · 2025-10-06T08:29:54Z

Codecov Report

❌ Patch coverage is 86.31179% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.14%. Comparing base (d24573b) to head (e555e88).

Files with missing lines	Patch %	Lines
tket/src/passes/borrow_squash.rs	87.64%	27 Missing and 4 partials ⚠️
tket/src/passes/guppy.rs	58.33%	3 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1159      +/-   ##
==========================================
+ Coverage   79.02%   79.14%   +0.11%     
==========================================
  Files         160      161       +1     
  Lines       20421    20678     +257     
  Branches    19489    19746     +257     
==========================================
+ Hits        16138    16365     +227     
- Misses       3294     3320      +26     
- Partials      989      993       +4

Flag	Coverage Δ
python	`92.65% <ø> (ø)`
qis-compiler	`100.00% <ø> (ø)`
rust	`78.48% <86.31%> (+0.12%)`	⬆️

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.

acl-cqc · 2025-10-13T11:23:43Z

tket/src/passes/squash_borrow.rs

+    }
+}
+
+/// Reasons we may fail to determine whether a node is a borrow/return.


TBH I do kinda think that these might be better all made into panics. As per comment on is_borrow_return, Ok(None) means "can't figure out this op, leave it be, continue only in areas excluding this op"; and it's quite hard to define over what (wider) scope an error would inhibit optimization (potentially including bits of the Hugr with no access to the same value but which by chance would have been optimized after the error was thrown, so were never run).

ss2165 · 2025-10-15T10:18:57Z

tket/src/passes/squash_borrow.rs

@@ -0,0 +1,708 @@
+//! Reorder and squash pairs of borrow and return nodes where possible.


Suggested change

//! Reorder and squash pairs of borrow and return nodes where possible.

//! Reorder and squash pairs of borrow and return operations on [`BorrowArray`] where possible.

ss2165 · 2025-10-15T10:22:40Z

tket/src/passes/squash_borrow.rs

+    ///   * `None` if the array's size is not statically known.
+    ///
+    /// Otherwise (if the type is not such an array), return `None`.
+    fn get_array_size(&self, ty: &Type) -> Option<Option<u64>>;


The nested option is a bit annoying for readability, could be worth a dedicated enum

Now just a bool, the assumption that in-range won't panic is not good if we have dynamic indexing among nested arrays 😢

ss2165 · 2025-10-15T10:25:48Z

tket/src/passes/squash_borrow.rs

+/// A pass for eliding (e.g. `BorrowArray`) reborrows of elements (with constant indices)
+/// along with the preceding return.


Suggested change

/// A pass for eliding (e.g. `BorrowArray`) reborrows of elements (with constant indices)

/// along with the preceding return.

/// A pass for eliding return followed by borrow on [`BorrowArray`] with matching constant index.

ss2165 · 2025-10-15T10:28:32Z

tket/src/passes/squash_borrow.rs

+impl<BR> BorrowSquashPass<BR> {
+    /// Add regions (subgraphs) in which to perform the pass.
+    ///
+    /// If no regions are specified, the pass is performed on all dataflow regions


Maybe Option<Vec<Node>> is a safer encapsulation of this? If for whatever reason the region list becomes empty (maybe I am popping and pushing to it somewhere else), I would assume nothing happens rather than everything.

The only way for the region list to become empty would be for that empty list to be passed in here...but done

yes I am talking about expected behaviour when called by external code (in some other composite pass for example)

tket/src/passes/squash_borrow.rs

ss2165 · 2025-10-15T11:11:15Z

tket/src/passes/borrow_squash.rs

+                .as_extension_op()
+                .is_some_and(|eop| eop.qualified_id() == "tket.quantum.CX"));
+        }
+        assert!(h.output_neighbours(cx1).all(|n| n == cx2));


could be nice to have this as an explicit assert-not-any before the pass

does assert that the output neighbours of both CX's were returns?

yeah fair enough, just preferred something more explicit but not important

ss2165 · 2025-10-15T11:13:11Z

tket/src/passes/squash_borrow.rs

+        matches!(op, OpType::LoadConstant(..))
+            || op
+                .as_extension_op()
+                .is_some_and(|op| ConvertOpDef::from_extension_op(op) == Ok(ConvertOpDef::itousize))


this feels flaky - future conversion ops wouldn't be covered. Would it not be better to rely on constant folding eliminating these beforehand?

Yes, done. Rather slows down the big-hugr test but it works...

maybe the test need not be so big?

let's wait for Agustin's integration tests and then maybe we can just drop some of these

ss2165 · 2025-10-15T11:15:04Z

tket/src/passes/borrow_squash.rs

+    Ok(Some((node, idx, is_br.action, is_br.borrow_from)))
+}
+
+fn wire_type(h: &impl HugrView<Node = Node>, w: Wire) -> Type {


https://docs.rs/hugr/latest/hugr/builder/trait.Dataflow.html#method.get_wire_type

That's on a builder::Dataflow, not a hugr ?!

ah woops that's annoying, still the implementation is on a hugr and saves iteration to find the port https://docs.rs/hugr-core/0.24.0/src/hugr_core/builder/build_traits.rs.html#555

Ah yes thanks! Done

ss2165 · 2025-10-15T11:18:25Z

tket/src/passes/squash_borrow.rs

+        let out_ty = wire_type(hugr, Wire::new(node, is_br.borrow_from.out));
+        if *array_ty != out_ty {
+            let array_ty = array_ty.clone();
+            return Err(BorrowAnalysisError::InconsistentArrayType { array_ty, out_ty });


should this be prevented by validation or could the rewrites in this pass induce this failure?

ss2165 · 2025-10-15T11:21:34Z

tket/src/passes/borrow_squash.rs

+    {
+        array = Wire::new(node, borrow_from.out); // for next iteration
+
+        match (action, borrowed.entry(index)) {


this match is very readable 👍

hugrbot · 2025-11-11T13:43:53Z

This PR contains breaking changes to the public Rust API.
Please deprecate the old API instead (if possible), or mark the PR with a ! to indicate a breaking change.

cargo-semver-checks summary

    Building tket v0.16.0 (current)
     Built [  40.559s] (current)
   Parsing tket v0.16.0 (current)
    Parsed [   0.099s] (current)
  Building tket v0.16.0 (baseline)
     Built [  41.140s] (baseline)
   Parsing tket v0.16.0 (baseline)
    Parsed [   0.083s] (baseline)
  Checking tket v0.16.0 -> v0.16.0 (assume minor change)
   Checked [   0.087s] 159 checks: 158 pass, 1 fail, 0 warn, 41 skip

--- failure enum_variant_added: enum variant added on exhaustive enum ---

Description:
A publicly-visible enum without #[non_exhaustive] has a new variant.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.45.0/src/lints/enum_variant_added.ron

Failed in:
variant NormalizeGuppyErrors:BorrowSquash in /home/runner/work/tket2/tket2/PR_BRANCH/tket/src/passes/guppy.rs:120

   Summary semver requires new major version: 1 major and 0 minor checks failed
  Finished [  83.900s] tket
  Building tket-qsystem v0.22.0 (current)
     Built [  42.116s] (current)
   Parsing tket-qsystem v0.22.0 (current)
    Parsed [   0.028s] (current)
  Building tket-qsystem v0.22.0 (baseline)
     Built [  42.155s] (baseline)
   Parsing tket-qsystem v0.22.0 (baseline)
    Parsed [   0.028s] (baseline)
  Checking tket-qsystem v0.22.0 -> v0.22.0 (assume minor change)
   Checked [   0.047s] 159 checks: 159 pass, 41 skip
   Summary no semver update required
  Finished [  86.527s] tket-qsystem

acl-cqc requested a review from a team as a code owner October 6, 2025 08:26

acl-cqc requested a review from ss2165 October 6, 2025 08:26

acl-cqc marked this pull request as draft October 8, 2025 15:58

acl-cqc changed the title ~~feat: BorrowSquashPass to elide redundant return-borrows~~ feat: BorrowSquashPass to elide redundant borrow/return ops Oct 8, 2025

acl-cqc changed the title ~~feat: BorrowSquashPass to elide redundant borrow/return ops~~ feat: BorrowSquashPass to elide redundant return-borrows Oct 8, 2025

acl-cqc force-pushed the acl/borrow_squash branch from b30d6a6 to 2724321 Compare October 8, 2025 16:02

acl-cqc changed the title ~~feat: BorrowSquashPass to elide redundant return-borrows~~ feat: BorrowSquashPass to elide redundant borrow/return ops Oct 8, 2025

acl-cqc mentioned this pull request Oct 12, 2025

feat: Add unsafe array take and put operations CQCL/guppylang#1165

Open

acl-cqc marked this pull request as ready for review October 13, 2025 11:15

acl-cqc commented Oct 13, 2025

View reviewed changes

ss2165 requested review from ss2165 and removed request for ss2165 October 14, 2025 13:40

acl-cqc mentioned this pull request Oct 14, 2025

Better Borrow-squashing by dataflow analysis #1176

Open

ss2165 reviewed Oct 15, 2025

View reviewed changes

lmondada mentioned this pull request Oct 27, 2025

feat: Analysis pass for BorrowSquash #1072

Closed

acl-cqc force-pushed the acl/borrow_squash branch from 3f5cf42 to 539aa15 Compare November 3, 2025 20:05

acl-cqc added 3 commits November 11, 2025 10:03

Add BorrowSquash pass + 3*test_files/ serialized Hugrs

5767a89

integration test

eada344

oops

b264751

acl-cqc force-pushed the acl/borrow_squash branch from 684f85c to b264751 Compare November 11, 2025 10:06

acl-cqc added 2 commits November 11, 2025 12:01

test updates...now fails with encoding bug

3edf898

fmt

340a8ee

acl-cqc mentioned this pull request Nov 11, 2025

feat: Don't translate usizes to pytket #1241

Merged

acl-cqc added 6 commits November 11, 2025 13:38

Merge remote-tracking branch 'origin/main' into HEAD

9a6ee32

Make test count gates before/after and with/out BorrowSquashPass

1a9205b

correct comment rebore -> borebo

1b67cdc

Test counts only CXs

f9809a6

squash_borrow => borrow_squash

ae0ceef

driveby: NormalizeGuppyPass: rename inline => inline_dfgs

53352a1

acl-cqc added 2 commits November 11, 2025 13:39

driveby: NormalizeGuppyPass: no need to map_err

c514985

Include in NormalizeGuppy

e555e88

acl-cqc force-pushed the acl/borrow_squash branch from bdc00d9 to e555e88 Compare November 11, 2025 13:39

		@@ -0,0 +1,708 @@
		//! Reorder and squash pairs of borrow and return nodes where possible.

		/// A pass for eliding (e.g. `BorrowArray`) reborrows of elements (with constant indices)
		/// along with the preceding return.

	/// A pass for eliding (e.g. `BorrowArray`) reborrows of elements (with constant indices)
	/// along with the preceding return.
	/// A pass for eliding return followed by borrow on [`BorrowArray`] with matching constant index.

feat: BorrowSquashPass to elide redundant borrow/return ops #1159

Are you sure you want to change the base?

feat: BorrowSquashPass to elide redundant borrow/return ops #1159

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugrbot commented Nov 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

acl-cqc commented Oct 6, 2025 •

edited

Loading

codecov bot commented Oct 6, 2025 •

edited

Loading