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.27%. Comparing base (69a126d) to head (a616a8e).
⚠️ Report is 2 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.27%   +0.19%     
==========================================
  Files         255      256       +1     
  Lines       48436    50444    +2008     
  Branches    43946    45967    +2021     
==========================================
+ Hits        40240    42005    +1765     
+ Misses       6103     6064      -39     
- Partials     2093     2375     +282

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

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 😅

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

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

+        signal: 2,
+        message: "Array element is already borrowed".to_string(),
+    };
+    static ref ERR_NOT_FREE: ConstError = ConstError {


ERR_NOT_BORROWED ? Is this the dual of the previous?

acl-cqc · 2025-09-24T12:38:21Z

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

+    }
+}
+
+/// A helper trait for customising the lowering of [`hugr_core::std_extensions::collections::array`]


Nit: ambiguous as to whether the array adjective refers just to types, or also CustomConsts and ops...perhaps customising the lowering of types, CustomConsts and ops, that are from array but if there's no good phrasing consider brackets ;)

acl-cqc · 2025-09-24T12:39:05Z

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

+/// A helper trait for customising the lowering of [`hugr_core::std_extensions::collections::array`]
+/// types, [`hugr_core::ops::constant::CustomConst`]s, and ops.
+///
+/// An `array<n, T>` is now lowered to a fat pointer `{ptr, usize}` that is allocated


Duplicates the module doc...can we crosslink instead

Although, one of these ought to describe the lowering of a borrow array rather than an array, i.e. with a bitmask too??

acl-cqc · 2025-09-24T12:46:43Z

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

+    Ok(array_v.into_struct_value())
+}
+
+/// Returns the underlying pointer and offset stored in a fat borrow array pointer.


pointer, mask and offset

acl-cqc · 2025-09-24T12:49:12Z

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

+        .build_bit_cast(mask_ptr, usize_t.ptr_type(AddressSpace::default()), "")?
+        .into_pointer_value();
+    // Initialise mask using memset
+    let memset_intrinsic = Intrinsic::find("llvm.memset").unwrap();


nit: not sure the intrinsic is helpful here, just inline?

Not sure what you mean? Why not use an intrinsics, it helps LLVM reason about the semantics of funtions?

Sorry, I mean inline the variable memset_intrinsic - I am sure using llvm.memset is the right thing to do!

I've done this

acl-cqc · 2025-09-24T12:50:34Z

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

+    } else {
+        ctx.iw_context().i8_type().const_zero()
+    };
+    let volatile = ctx.iw_context().bool_type().const_zero().into();


What is this?....an argument to pass to memset, i.e. that the mask is not volatile?

Yes exactly: https://llvm.org/docs/LangRef.html#llvm-memset-intrinsics

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

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

Apologies for force-push but I made sure the old/new commits were identical by git diff. The force-push has had the effect of removing, from the commit-list of this PR, both #2568 and all merges with main.

The first commit bdc4795 duplicates hugr-llvm/src/collections/array.rs to borrow_array.rs. After that, the remaining changes are "only":

.../std_extensions/collections/borrow_array.rs  |    2 +-
 hugr-llvm/src/emit/func.rs                      |   69 +-
 .../src/extension/collections/borrow_array.rs   | 1612 ++++++++++++++---
 ...borrow_array__test__emit_all_ops@llvm14.snap |  Bin 0 -> 16356 bytes
 ...__test__emit_all_ops@pre-mem2reg@llvm14.snap |  Bin 0 -> 27469 bytes
 ...ow_array__test__emit_array_value@llvm14.snap |  Bin 0 -> 1303 bytes
 ...st__emit_array_value@pre-mem2reg@llvm14.snap |  Bin 0 -> 1685 bytes
 ...__borrow_array__test__emit_clone@llvm14.snap |  Bin 0 -> 4753 bytes
 ...ay__test__emit_clone@pre-mem2reg@llvm14.snap |  Bin 0 -> 5705 bytes
 ...ns__borrow_array__test__emit_get@llvm14.snap |  Bin 0 -> 3190 bytes
 ...rray__test__emit_get@pre-mem2reg@llvm14.snap |  Bin 0 -> 4379 bytes
 11 files changed, 1431 insertions(+), 252 deletions(-)`

…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 😅

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

acl-cqc · 2025-10-06T15:40:37Z

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

    builder: &Builder<'c>,
    array_v: BasicValueEnum<'c>,
-) -> Result<(PointerValue<'c>, IntValue<'c>)> {
+) -> Result<(PointerValue<'c>, PointerValue<'c>, IntValue<'c>)> {


I lean towards a struct BArray(Fat)PointerFields<'a> { array_ptr: PointerValue<'a>, mask_ptr: PointerValue<'a>, array_offset: IntValue<'a> } for return value here and arguments to build_fat_pointer above. Any objection? I can do that but would call the first elems_ptr.

No, I think there is also the option to implement BasicValue for this struct (similar to how we do sum types)

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

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

+        .build_bit_cast(mask_ptr, usize_t.ptr_type(AddressSpace::default()), "")?
+        .into_pointer_value();
+    // Initialise mask using memset
+    let memset_intrinsic = Intrinsic::find("llvm.memset").unwrap();


I've done this

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

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?)

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 6 commits October 6, 2025 18:25

comments

437762e

Avoid a match using .as_ref().map::<&dyn BasicValue,_>

1775208

rename toplevel funcs to be consistent

207b254

Inline variable 'memset_intrinsic'; outline i8t with comment

f729f34

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 3 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

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!

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!

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

codecov bot commented Sep 22, 2025 •

edited

Loading

ss2165 commented Oct 6, 2025 •

edited

Loading

acl-cqc commented Oct 6, 2025 •

edited

Loading