Skip to content

feat!: Improved array lowering #2109

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
May 6, 2025
Merged

feat!: Improved array lowering #2109

merged 11 commits into from
May 6, 2025

Conversation

mark-koch
Copy link
Contributor

@mark-koch mark-koch commented Apr 23, 2025

Feature branch for improved array lowering.

  • The old array type is now called value_array and lives in a separate extension
  • The default array is now a linear type with additional clone and discard operations
  • To avoid code duplication, array operations and values are now defined generically over a new ArrayKind trait that is instantiated with Array (the linear one) and VArray (the copyable one) to generate the array and value_array extensions
  • An array<n, T> is now lowered to a fat pointer {ptr, usize} where ptr is a heap allocated pointer of size at least n * sizeof(T) and the usize is an offset pointing to the first element (i.e. the first element is at ptr + offset * sizeof(T)). The rational behind the additional offset is the pop_left operation which bumps the offset instead of mutating the pointer. This way, we can still free the original pointer when the array is discarded after a pop.

Tracked PRs:

BREAKING CHANGE: std.collections.array is now a linear type, even if the contained elements are copyable. Use the new std.collections.value_array for an array with the previous copyable semantics.

BREAKING CHANGE: std.collections.array.get now also returns the passed array as an extra output

BREAKING CHANGE: ArrayOpBuilder was moved from hugr_core::std_extensions::collections::array::op_builder to hugr_core::std_extensions::collections::array.

* The old `array` type is now called `value_array` and lives in a
separate extension
* The default `array` is now a linear type. Additional `clone` and
`discard` operations follow in a separate PR
* To avoid code duplication, array operations and values are now defined
generically over a new `ArrayKind` trait that is instantiated with
`Array` (the linear one) and `VArray` (the copyable one) to generate the
`array` and `value_array` extensions

Closes #2066

BREAKING CHANGE: `std.collections.array` is now a linear type, even if
the contained elements are copyable. Use the new
`std.collections.value_array` for an array with the previous copyable
semantics.
@hugrbot
Copy link
Collaborator

hugrbot commented Apr 23, 2025

This PR contains breaking changes to the public Rust API.

cargo-semver-checks summary

--- failure enum_missing: pub enum removed or renamed ---

Description:
A publicly-visible enum cannot be imported by its prior path. A `pub use` may have been removed, or the enum itself may have been renamed or removed entirely.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/enum_missing.ron

Failed in:
enum hugr_core::std_extensions::collections::array::ArrayOpDef, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/std_extensions/collections/array/array_op.rs:27

--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/struct_missing.ron

Failed in:
struct hugr_core::std_extensions::collections::array::ArrayScan, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/std_extensions/collections/array/array_scan.rs:134
struct hugr_core::std_extensions::collections::array::ArrayOpDefIter, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/std_extensions/collections/array/array_op.rs:24
struct hugr_core::std_extensions::collections::array::ArrayValue, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/std_extensions/collections/array.rs:41
struct hugr_core::std_extensions::collections::array::ArrayRepeat, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/std_extensions/collections/array/array_repeat.rs:101
struct hugr_core::std_extensions::collections::array::ArrayRepeatDef, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/std_extensions/collections/array/array_repeat.rs:22
struct hugr_core::std_extensions::collections::array::ArrayOp, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/std_extensions/collections/array/array_op.rs:236
struct hugr_core::std_extensions::collections::array::ArrayScanDef, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/std_extensions/collections/array/array_scan.rs:24

--- failure trait_added_supertrait: non-sealed trait added new supertraits ---

Description:
A non-sealed trait added one or more supertraits, which breaks downstream implementations of the trait
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#generic-bounds-tighten
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_added_supertrait.ron

Failed in:
trait hugr_core::std_extensions::collections::array::ArrayOpBuilder gained GenericArrayOpBuilder in file /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/std_extensions/collections/array.rs:174

--- failure trait_missing: pub trait removed or renamed ---

Description:
A publicly-visible trait cannot be imported by its prior path. A `pub use` may have been removed, or the trait itself may have been renamed or removed entirely.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_missing.ron

Failed in:
trait hugr_core::std_extensions::collections::array::op_builder::ArrayOpBuilder, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/std_extensions/collections/array/op_builder.rs:13

--- failure trait_removed_supertrait: supertrait removed or renamed ---

Description:
A supertrait was removed from a trait. Users of the trait can no longer assume it can also be used like its supertrait.
      ref: https://doc.rust-lang.org/reference/items/traits.html#supertraits
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_removed_supertrait.ron

Failed in:
supertrait hugr_core::builder::Dataflow of trait ArrayOpBuilder in file /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/std_extensions/collections/array.rs:174

--- failure function_parameter_count_changed: pub fn parameter count changed ---

Description:
A publicly-visible function now takes a different number of parameters.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/function_parameter_count_changed.ron

Failed in:
hugr_llvm::extension::collections::array::emit_repeat_op now takes 4 parameters instead of 3, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-llvm/src/extension/collections/array.rs:803
hugr_llvm::extension::collections::array::emit_scan_op now takes 6 parameters instead of 5, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-llvm/src/extension/collections/array.rs:831

--- failure function_missing: pub fn removed or renamed ---

Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/function_missing.ron

Failed in:
function hugr_passes::replace_types::handlers::linearize_array, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-passes/src/replace_types/handlers.rs:73

Copy link

codecov bot commented Apr 23, 2025

Codecov Report

Attention: Patch coverage is 80.25806% with 306 lines in your changes missing coverage. Please review.

Project coverage is 82.90%. Comparing base (20081dd) to head (4edbc49).
Report is 1 commits behind head on release-rs-v0.16.0.

Files with missing lines Patch % Lines
...core/src/std_extensions/collections/value_array.rs 35.24% 78 Missing and 1 partial ⚠️
hugr-llvm/src/extension/collections/array.rs 80.85% 3 Missing and 69 partials ⚠️
...rc/std_extensions/collections/array/array_value.rs 59.72% 28 Missing and 1 partial ⚠️
hugr-passes/src/replace_types/handlers.rs 63.88% 26 Missing ⚠️
...rc/std_extensions/collections/array/array_clone.rs 86.17% 11 Missing and 2 partials ⚠️
...d_extensions/collections/array/array_conversion.rs 87.37% 11 Missing and 2 partials ⚠️
.../std_extensions/collections/array/array_discard.rs 85.05% 11 Missing and 2 partials ⚠️
...src/std_extensions/collections/array/op_builder.rs 92.03% 1 Missing and 8 partials ⚠️
hugr-passes/src/linearize_array.rs 94.00% 9 Missing ⚠️
hugr-py/src/hugr/std/collections/value_array.py 81.63% 9 Missing ⚠️
... and 6 more
Additional details and impacted files
@@                  Coverage Diff                   @@
##           release-rs-v0.16.0    #2109      +/-   ##
======================================================
- Coverage               83.05%   82.90%   -0.15%     
======================================================
  Files                     219      227       +8     
  Lines                   41385    42288     +903     
  Branches                37533    38387     +854     
======================================================
+ Hits                    34372    35060     +688     
- Misses                   5186     5360     +174     
- Partials                 1827     1868      +41     
Flag Coverage Δ
python 85.64% <82.00%> (-0.06%) ⬇️
rust 82.62% <80.20%> (-0.16%) ⬇️

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.

@mark-koch mark-koch changed the base branch from main to release-rs-v0.16.0 April 24, 2025 12:40
mark-koch and others added 5 commits April 24, 2025 14:21
Since the array type is now linear following #2097, we need to change
the signature of the `array.get` op to give the passed array back to the
user.

BREAKING CHANGE: `std.collections.array.get` now also returns the passed
array as an extra output
Closes #2067

An `array<n, T>` is now lowered to a struct `{ptr, usize}` where `ptr`
is a heap allocated pointer of size at least `n * sizeof(T)` and the
`usize` is an offset pointing to the first element (i.e. the first
element is at `ptr + offset * sizeof(T)`).

The rational behind the additional offset is the `pop_left` operation
which bumps the offset instead of mutating the pointer. This way, we can
still free the original pointer when the array is discarded after a pop.

BREAKING CHANGE: Removed old lowering for value arrays
mark-koch and others added 4 commits May 6, 2025 12:01
* Renamed `ArrayOpBuilder` trait to `GenericOpBuilder` whose methods are
generic over the array implementation
* Defined new `ArrayOpBuilder` and `VArrayOpBuilder` traits that are
implemented in terms of the generic version. I moved those into the
`array` and `value_array` modules respectively, so they don't come into
scope at the same time
* Moved `all_array_ops` ops function out of the tests module so we can
reuse it across other tests (in particular getting rid of the code
duplication in `hugr-llvm::extension::collections::array::test`

BREAKING CHANGE: `ArrayOpBuilder` was moved from
`hugr_core::std_extensions::collections::array::op_builder` to
`hugr_core::std_extensions::collections::array`
@mark-koch mark-koch marked this pull request as ready for review May 6, 2025 11:49
@mark-koch mark-koch requested a review from a team as a code owner May 6, 2025 11:50
@mark-koch mark-koch requested review from acl-cqc and ss2165 and removed request for acl-cqc May 6, 2025 11:50
Copy link
Member

@ss2165 ss2165 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets gooooo

@mark-koch mark-koch added this pull request to the merge queue May 6, 2025
Merged via the queue into release-rs-v0.16.0 with commit 731081f May 6, 2025
26 checks passed
@mark-koch mark-koch deleted the feat/arrays branch May 6, 2025 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants