feat: LLVM lowering for borrow arrays using bitmasks #2574

mark-koch · 2025-09-22T16:45:18Z

The first two commits just copy the existing lowering. The changes to the lowering lgic are implemented in 888e457.

codecov · 2025-09-22T16:50:12Z

Codecov Report

❌ Patch coverage is 91.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.32%. Comparing base (69a126d) to head (6cf427b).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
hugr-llvm/src/emit/func.rs	91.48%	1 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2574      +/-   ##
==========================================
+ Coverage   83.07%   83.32%   +0.25%     
==========================================
  Files         255      256       +1     
  Lines       48436    50488    +2052     
  Branches    43946    46011    +2065     
==========================================
+ Hits        40240    42071    +1831     
+ Misses       6103     6064      -39     
- Partials     2093     2353     +260

Flag	Coverage Δ
python	`91.53% <ø> (-0.03%)`	⬇️
rust	`82.53% <91.66%> (+0.31%)`	⬆️

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-09-24T12:30:33Z

hugr-llvm/src/emit/func.rs

+    let func = ctx
+        .get_current_module()
+        .get_function(func_name)
+        .map(Ok::<_, anyhow::Error>)


Nit: Option of Result, etc., is a bit hard to figure out. How about let func = match ctx.get_current_module().get_function(func_name) { Some(f) => f, None => { .... } } which moves your closure into the body and removes the nested ?.

Or perhaps consider let func = if let Some(f) = ctx.blah() {f} else {....}

tatiana-s · 2025-09-24T12:41:53Z

hugr-llvm/src/extension/collections/borrow_array.rs

+}
+
+impl<'a, H: HugrView<Node = Node> + 'a> CodegenExtsBuilder<'a, H> {
+    /// Add a [`BorrowArrayCodegenExtension`] to the given [`CodegenExtsBuilder`] using `ccg`


nit: comments should be the other way round?

acl-cqc

Well I thought I'd take a look at this, but +4,000 lines and I'm stopping a fraction of the way through.... the PR we are thinking this replaces is a third as long as this, right? How many opportunities are there for bugs here? (ReplaceTypes BorrowArray has the benefit that we get type-checking of the output Hugr by the hugr type system, too)

acl-cqc · 2025-09-24T12:31:27Z

hugr-llvm/src/emit/func.rs

+/// The first time this helper is called with a given function name, a function is built
+/// using the provided closure. Future invocations with the same name will just emit calls
+/// to this function.
+pub fn outline_into_function<'c, H: HugrView<Node = Node>, const N: usize>(


Perhaps get_or_make_function

acl-cqc · 2025-09-24T12:33:00Z

hugr-llvm/src/extension/collections/borrow_array.rs

+//! to at least `n * sizeof(T)` bytes. The extra `usize` is an offset pointing to the
+//! first element, i.e. the first element is at address `ptr + offset * sizeof(T)`.
+//!
+//! The rational behind the additional offset is the `pop_left` operation which bumps


I like this explanation, but is there no push_left / unpop ?

Also, rational -> rationale

is there no push_left / unpop ?

No, as this would require reallocating the array in general

acl-cqc · 2025-09-24T12:33:29Z

hugr-llvm/src/extension/collections/borrow_array.rs

+//! the offset instead of mutating the pointer. This way, we can still free the original
+//! pointer when the array is discarded after a pop.
+//!
+//! We provide utility functions [`array_fat_pointer_ty`], [`build_barray_fat_pointer`], and


Why are some of these barray and others just array ?

Because I forgot to rename those after copy pasting 😅

hugr-llvm/src/extension/collections/borrow_array.rs

tatiana-s · 2025-09-25T09:49:08Z

Well I thought I'd take a look at this, but +4,000 lines and I'm stopping a fraction of the way through.... the PR we are thinking this replaces is a third as long as this, right? How many opportunities are there for bugs here? (ReplaceTypes BorrowArray has the benefit that we get type-checking of the output Hugr by the hugr type system, too)

But most of this PR just copies the existing array lowering (including many snapshots) so it is only the bit mask code that is new which is a lot less than the type replacement

ss2165 · 2025-10-06T11:28:03Z

~~Please include #2609 before merging~~ done

…n closure

acl-cqc

Nearly there....done a bunch of mods, hope that's ok, a few questions here

acl-cqc · 2025-10-06T15:09:26Z

hugr-llvm/src/emit/func.rs

+        Some(func) => func,
+        None => {
+            let arg_tys = args.iter().map(|v| v.get_type().into()).collect_vec();
+            let sig = match ret_type {


bizarre here that BasicTypeEnum and VoidType define fn_type as two completely unrelated methods! But yeah I don't see a better way :(

acl-cqc · 2025-10-06T15:16:44Z

hugr-llvm/src/extension/collections/borrow_array.rs

+pub trait BorrowArrayCodegen: Clone {
+    /// Emit instructions to halt execution with the error `err`.
+    ///
+    /// This should be consistent with the panic implementation from the [PreludeCodegen].


So what you end up doing is parametrizing the struct that implements BorrowArrayCodegen by something that implements PreludeCodegen for this.

I guess the alternative is to define a PanicCodegen super-trait to both PreludeCodegen and BorrowArrayCodegen. The default impl in PreludeCodegen would then go there. Did you consider this? I don't really understand well enough how/where this is all used to know the advantages/disadvantages, but I could do/try that if you think it'd be better?

Did you consider this?

No, I just went with this approach since we also used it for e.g. IntCodegenExtension. But I wouldn't mind changing to the super trait approach

acl-cqc · 2025-10-06T15:19:15Z

hugr-llvm/src/extension/collections/borrow_array.rs

    }

-    /// Emit a [`hugr_core::std_extensions::collections::array::ArrayOp`].
+    /// Emit a [`hugr_core::std_extensions::collections::borrow_array::BArrayOp`].


Should this be called emit_barray_op? and similarly ...._unsafe_op and emit_barray_{clone, discard, repeat} etc. below?

If we keep them as array that would ease recombining via trait BorrowArrayCodegen: ArrayCodegen although I'm not sure whether the change in parametrization would prevent that - I think you might need trait GenericArrayCodegen<AK : ArrayKind> which then makes default-impls hard

Yes, this should all be barray, silly copy paste mistakes sorry 😅

Did the top-levels, kept the trait. Figuring out how to recombine some of this code would be good (but left for later) - I'm not sure how to atm!

acl-cqc · 2025-10-06T15:36:36Z

hugr-llvm/src/extension/collections/borrow_array.rs

        &ctx.typing_session(),
        ptr.get_type().get_element_type().try_into().unwrap(),
    );
    let array_v = array_ty.get_poison();
    let array_v = ctx
        .builder()
        .build_insert_value(array_v, ptr.as_basic_value_enum(), 0, "")?;
+    let array_v =
+        ctx.builder()
+            .build_insert_value(array_v, mask_ptr.as_basic_value_enum(), 1, "")?;


The "" for name seems standard throughout the repo, but would strings like "elem_ptr", "bitmask_ptr" and "offset" help with debugging or cause problems? (There are more things to have to look up here...)

We use strings for build_extract_value after all!

Yes, this would probably be a good thing to do

hugr-llvm/src/extension/collections/borrow_array.rs

acl-cqc · 2025-10-06T15:55:28Z

hugr-llvm/src/extension/collections/borrow_array.rs

+        )
+        .unwrap();
+    let val = if value {
+        ctx.iw_context().i8_type().const_all_ones()


I've documented that this argument is always an i8t after breaking everything by trying usize_ty(&ctx.typing_session()) here instead ;)

acl-cqc · 2025-10-06T16:06:15Z

hugr-llvm/src/extension/collections/borrow_array.rs

+    let block_size = usize_t.const_int(usize_t.get_bit_width() as u64, false);
+    let builder = ctx.builder();
+    let block_idx = builder.build_int_unsigned_div(idx, block_size, "")?;
+    let block_ptr = unsafe { builder.build_in_bounds_gep(mask_ptr, &[block_idx], "")? };


Rust code that builds unsafe binary code is itself marked unsafe....!!! Lol, ok

acl-cqc · 2025-10-06T16:24:13Z

hugr-llvm/src/extension/collections/borrow_array.rs

+    let bit = builder.build_int_truncate(block_shifted, ctx.iw_context().bool_type(), "")?;
+
+    let borrowed_bb =
+        ctx.build_positioned_new_block("elem_borrowed", None, |ctx, borrowed_bb| {


I do wonder if these closures are, and this much generality is, a bit OTT...I switched out the if...get_terminator check to put the build_return(None) in the caller, that saved a bit. But passing two Option<&str> panic messages would probably save a bunch more, or one panic message and a bool 'expected_result' (so it's check_bitmask_eq) which would allow a for loop to build two basic blocks...any objections to the latter approach?

Also thinking that if we kept a closure (for the success block) we could put the mask-flipping into the same block, which would avoid redoing all the block-index-calculation stuff in build_mask_flip (both at runtime, and duplicating the Rust code here)

any objections to the latter approach?

No, that sounds good 👍

we could put the mask-flipping into the same block

Also happy with this

Done. I'm almost sad there are only execution tests because now we don't see how much smaller the code is ;-)

Done with an enum MaskCheck which specifies which value to panic on and whether to flip

acl-cqc · 2025-10-06T17:14:42Z

hugr-llvm/src/extension/collections/borrow_array.rs

-    })?;
-
-    let head_block = ctx.build_positioned_new_block("", Some(body_block), |ctx, bb| {
+    let (body_start_block, body_end_block, val) =


The extra values here are to allow the closure to add new blocks/branches/controlflow/etc. rather than only being able to emit instructions inside one block, right??

So does this build_loop generalize the old one (and we can remove the old from from array.rs?)

Yes, we should be able to remove the old one (and maybe move the new one somewhere else)

There's a third in stack_array.rs and moreover, these are all internal, so could be done later. Hence left for now.

… inside

hugr-llvm/src/extension/collections/borrow_array.rs

… pass

…putation

…moning up

… MaskCheck)

acl-cqc · 2025-10-08T10:01:04Z

Ok I guess this is now a joint PR ;-), hopefully this link shows just the changes I've made to Mark's original: https://github.com/CQCL/hugr/pull/2574/files/439f09c27e6591c125088000e03d8556b88fef2b..6cf427bee12c8f65fa35e3374cf4b47c5c9dd9ed

ss2165

Just some minor docstring/refactor suggestions

ss2165 · 2025-10-08T10:21:36Z

hugr-llvm/src/extension/collections/borrow_array.rs

-        go(ctx, block)
-    })
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+enum MaskCheck {


docstrings on these would be good

ss2165 · 2025-10-08T10:22:02Z

hugr-llvm/src/extension/collections/borrow_array.rs


-/// Emits instructions to flip the borrowed mask flag for a given index.
-fn build_mask_flip<'c, H: HugrView<Node = Node>>(
+fn check_mask_elem<'c, H: HugrView<Node = Node>>(


docstring 🙏

this would seem to be a natural method on MaskCheck

mark-koch requested a review from tatiana-s September 22, 2025 16:45

mark-koch requested a review from a team as a code owner September 22, 2025 16:45

mark-koch requested review from acl-cqc and removed request for a team and acl-cqc September 22, 2025 16:45

acl-cqc reviewed Sep 24, 2025

View reviewed changes

tatiana-s reviewed Sep 24, 2025

View reviewed changes

acl-cqc reviewed Sep 24, 2025

View reviewed changes

mark-koch force-pushed the feat/barray-llvm branch 4 times, most recently from d82bb30 to ba4df3d Compare September 29, 2025 14:05

Base automatically changed from test/panic to main September 29, 2025 15:46

mark-koch requested review from tatiana-s and acl-cqc September 30, 2025 08:57

mark-koch and others added 11 commits October 6, 2025 15:21

Make copy of existing array lowering

bdc4795

Rename everything

5bc67e3

Implement bitmask lowering

fe5d575

Handle 0 sized arrays

b23b593

Clippy

b3d5b8e

Stop using lazy_static

877771c

Fix docs

924a6d3

Refactor and rename outline_into_function

2d19e2f

Rename ERR_NOT_FREE to ERR_NOT_BORROWED

cb58a3a

Add codegen for conversion ops

56eee57

refactor: allow non-void get_or_make_function (#2609)

439f09c

acl-cqc force-pushed the feat/barray-llvm branch from 91f42b2 to 439f09c Compare October 6, 2025 14:22

acl-cqc added 2 commits October 6, 2025 18:25

inspect_mask_idx closure adds terminator

e3ee86d

inspect_mask_blocks takes expected value and error message rather tha…

975771e

…n closure

acl-cqc reviewed Oct 6, 2025

View reviewed changes

acl-cqc added 4 commits October 7, 2025 08:53

rename inspect_mask_blocks, doc

a33d99c

update snapshots due to renaming of blocks

aaac55c

also pass MaskInfo into build_mask_padding

a616a8e

check_all_mask_blocks_eq -> check_all_mask_eq with build_mask_padding…

cab33fa

… inside

mark-koch commented Oct 7, 2025

View reviewed changes

hugr-llvm/src/extension/collections/borrow_array.rs Outdated Show resolved Hide resolved

acl-cqc force-pushed the feat/barray-llvm branch from 2d069c9 to 95b354a Compare October 7, 2025 12:15

acl-cqc added 3 commits October 7, 2025 13:47

switch triple to struct BArrayFatPtrComponents

72e7c54

A load more discard_all_borrowed_panic tests...all passing

4cc2bca

Test returning just one index to an array of all-borrowed...still all…

9b6299c

… pass

acl-cqc force-pushed the feat/barray-llvm branch from 95b354a to 5be9249 Compare October 7, 2025 13:38

acl-cqc added 3 commits October 7, 2025 15:37

more tests, finally demonstrating the problem(s?)

073d039

fix shifting in one direction only, with llvm test

202dc48

fix: reorder loads/stores in build_mask_padding

d5d5999

acl-cqc force-pushed the feat/barray-llvm branch from 5be9249 to d5d5999 Compare October 7, 2025 14:59

acl-cqc added 10 commits October 7, 2025 21:20

Fix accessing/padding beyond end of mask

4a64ae4

trivial refactor: pass last_idx into build_mask_padding, avoids recom…

6c45cbf

…putation

refactor: split build_mask_padding into two calls

e619155

refactor: build_mask_padding1d tests value before direction, some com…

bf4b6d6

…moning up

build_barray_alloc: do not return mask_ptr

b056e4d

build_barray_fat_pointer takes BArrayFatPointerComponents

b80bc0e

build_(idx_(free,not_borrowed)_check,mask_flip) -> check_mask_eq(enum…

5ca60ce

… MaskCheck)

Avoid xor in mask-check methods by flipping bbs; update more snapshots

dc99387

common up array allocation w/bit-cast

c2cdf79

check_mask_idx -> check_mask_elem, does not check the index is correct

6cf427b

acl-cqc requested a review from ss2165 October 8, 2025 11:01

ss2165 approved these changes Oct 8, 2025

View reviewed changes

feat: LLVM lowering for borrow arrays using bitmasks #2574

Are you sure you want to change the base?

feat: LLVM lowering for borrow arrays using bitmasks #2574

Conversation

mark-koch commented Sep 22, 2025

Uh oh!

codecov bot commented Sep 22, 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!

acl-cqc 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tatiana-s commented Sep 25, 2025

Uh oh!

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

Uh oh!

Uh oh!

acl-cqc 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!

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

codecov bot commented Sep 22, 2025 •

edited

Loading

ss2165 commented Oct 6, 2025 •

edited

Loading