-
Notifications
You must be signed in to change notification settings - Fork 8
feat: Add array linearisation pass #2125
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/arrays #2125 +/- ##
==============================================
Coverage ? 83.33%
==============================================
Files ? 227
Lines ? 43078
Branches ? 39131
==============================================
Hits ? 35901
Misses ? 5323
Partials ? 1854
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
// are now linear, we'd have to replace it with a `set`. But what | ||
// should we put in??? For now, let's just error out and make sure | ||
// we're not emitting `get`s for nested value arrays. | ||
if op_def == ArrayOpDef::get && !args[1].as_type().unwrap().copyable() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check avoids get
s for all linear types, not just nested value arrays, right? The error message should probably reflect that. It'd also be good to document this limitation in some doc comment on the pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validity checking should ensure that the type was copyable before we ran the pass. The only way for the element to become linear is if it used to contain a value_array
that we turned into a linear array
.
I'll add it to the doc 👍
pub struct LinearizeArrayPass(ReplaceTypes); | ||
|
||
impl Default for LinearizeArrayPass { | ||
fn default() -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's convention in Rust to have a new()
method in this case as well.
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: * #2097 (closes #2066) * #2100 * #2101 * #2110 * #2112 (closes #2067) * #2119 * #2125 (closes #2124) 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`.
Closes #2124