Skip to content
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.

Sign up for GitHub

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

feat: Lists and extension sets with splicing #1657

Merged
merged 4 commits into from
Nov 27, 2024
Merged

feat: Lists and extension sets with splicing #1657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f193426
Lists and extension sets with splicing
zrho Nov 14, 2024
cf94a88
`export_type_row_with_tail`
zrho Nov 21, 2024
7823ac9
Comments
zrho Nov 21, 2024
3905b81
ListParts, ExtSetParts and fixed import bug that would lead to wrong …
zrho Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
167 changes: 69 additions & 98 deletions hugr-core/src/export.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,48 +509,24 @@ impl<'a> Context<'a> {
/// like for the other nodes since the ports are control flow ports.
pub fn export_block_signature(&mut self, block: &DataflowBlock) -> model::TermId {
let inputs = {
let mut inputs = BumpVec::with_capacity_in(block.inputs.len(), self.bump);
for input in block.inputs.iter() {
inputs.push(self.export_type(input));
}
let inputs = self.make_term(model::Term::List {
items: inputs.into_bump_slice(),
tail: None,
});
let inputs = self.export_type_row(&block.inputs);
let inputs = self.make_term(model::Term::Control { values: inputs });
self.make_term(model::Term::List {
items: self.bump.alloc_slice_copy(&[inputs]),
tail: None,
parts: self.bump.alloc_slice_copy(&[model::ListPart::Item(inputs)]),
})
};

let tail = {
let mut tail = BumpVec::with_capacity_in(block.other_outputs.len(), self.bump);
for other_output in block.other_outputs.iter() {
tail.push(self.export_type(other_output));
}
self.make_term(model::Term::List {
items: tail.into_bump_slice(),
tail: None,
})
};
let tail = self.export_type_row(&block.other_outputs);

let outputs = {
let mut outputs = BumpVec::with_capacity_in(block.sum_rows.len(), self.bump);
for sum_row in block.sum_rows.iter() {
let mut variant = BumpVec::with_capacity_in(sum_row.len(), self.bump);
for typ in sum_row.iter() {
variant.push(self.export_type(typ));
}
let variant = self.make_term(model::Term::List {
items: variant.into_bump_slice(),
tail: Some(tail),
});
outputs.push(self.make_term(model::Term::Control { values: variant }));
let variant = self.export_type_row_with_tail(sum_row, Some(tail));
let control = self.make_term(model::Term::Control { values: variant });
outputs.push(model::ListPart::Item(control));
}
self.make_term(model::Term::List {
items: outputs.into_bump_slice(),
tail: None,
parts: outputs.into_bump_slice(),
})
};

Expand Down Expand Up @@ -772,10 +748,12 @@ impl<'a> Context<'a> {
TypeArg::String { arg } => self.make_term(model::Term::Str(self.bump.alloc_str(arg))),
TypeArg::Sequence { elems } => {
// For now we assume that the sequence is meant to be a list.
let items = self
.bump
.alloc_slice_fill_iter(elems.iter().map(|elem| self.export_type_arg(elem)));
self.make_term(model::Term::List { items, tail: None })
let parts = self.bump.alloc_slice_fill_iter(
elems
.iter()
.map(|elem| model::ListPart::Item(self.export_type_arg(elem))),
);
self.make_term(model::Term::List { parts })
}
TypeArg::Extensions { es } => self.export_ext_set(es),
TypeArg::Variable { v } => self.export_type_arg_var(v),
Expand All @@ -798,32 +776,53 @@ impl<'a> Context<'a> {
pub fn export_sum_type(&mut self, t: &SumType) -> model::TermId {
match t {
SumType::Unit { size } => {
let items = self.bump.alloc_slice_fill_iter((0..*size).map(|_| {
self.make_term(model::Term::List {
items: &[],
tail: None,
})
let parts = self.bump.alloc_slice_fill_iter((0..*size).map(|_| {
model::ListPart::Item(self.make_term(model::Term::List { parts: &[] }))
}));
let list = model::Term::List { items, tail: None };
let variants = self.make_term(list);
let variants = self.make_term(model::Term::List { parts });
self.make_term(model::Term::Adt { variants })
}
SumType::General { rows } => {
let items = self
.bump
.alloc_slice_fill_iter(rows.iter().map(|row| self.export_type_row(row)));
let list = model::Term::List { items, tail: None };
let parts = self.bump.alloc_slice_fill_iter(
rows.iter()
.map(|row| model::ListPart::Item(self.export_type_row(row))),
);
let list = model::Term::List { parts };
let variants = { self.make_term(list) };
self.make_term(model::Term::Adt { variants })
}
}
}

pub fn export_type_row<RV: MaybeRV>(&mut self, t: &TypeRowBase<RV>) -> model::TermId {
let mut items = BumpVec::with_capacity_in(t.len(), self.bump);
items.extend(t.iter().map(|row| self.export_type(row)));
let items = items.into_bump_slice();
self.make_term(model::Term::List { items, tail: None })
#[inline]
pub fn export_type_row<RV: MaybeRV>(&mut self, row: &TypeRowBase<RV>) -> model::TermId {
self.export_type_row_with_tail(row, None)
}

pub fn export_type_row_with_tail<RV: MaybeRV>(
&mut self,
row: &TypeRowBase<RV>,
tail: Option<model::TermId>,
) -> model::TermId {
let mut parts = BumpVec::with_capacity_in(row.len() + tail.is_some() as usize, self.bump);

for t in row.iter() {
match t.as_type_enum() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider items.push(match t.as_type_enum() { .... }) or even items.push(if let TypeEnum::RowVar(var)=t.as_type_enum() {...} else {...})

Copy link
Contributor

Choose a reason for hiding this comment

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

A shame there is no from_sized_iter_in so you could create the BumpVec and fill it in one go!

Copy link
Contributor

Choose a reason for hiding this comment

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

Does self.bump.alloc_slice_fill_iter do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a few places where Bump::alloc_slice_fill_iter would in principle work. I've kept the BumpVec at the moment since I am leaving open the option that export may be fallible and return a Result, in which case it'd be easier to change the BumpVec option.

Also Bump::alloc_slice_fill_iter takes an ExactSizeIterator which we do not have in some cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also Bump::alloc_slice_fill_iter takes an ExactSizeIterator which we do not have in some cases.

Ah ok - I wondered whether there was a problem something like that, and then when I saw that every Iterator has a size_hint I thought maybe it'd be ok anyway (i.e. sizing with capacity on a best-effort basis). If alloc_slice_fill_iter of a non-ExactSizeIterator doesn't compile...then never mind, BumpVec it'll have to be.

TypeEnum::RowVar(var) => {
parts.push(model::ListPart::Splice(self.export_row_var(var.as_rv())));
}
_ => {
parts.push(model::ListPart::Item(self.export_type(t)));
}
}
}

if let Some(tail) = tail {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming export_type_row to export_type_row_with_tail and then adding export_type_row that just adds the None

parts.push(model::ListPart::Splice(tail));
}

let parts = parts.into_bump_slice();
self.make_term(model::Term::List { parts })
}

/// Exports a `TypeParam` to a term.
Expand Down Expand Up @@ -855,12 +854,12 @@ impl<'a> Context<'a> {
self.make_term(model::Term::ListType { item_type })
}
TypeParam::Tuple { params } => {
let items = self.bump.alloc_slice_fill_iter(
let parts = self.bump.alloc_slice_fill_iter(
params
.iter()
.map(|param| self.export_type_param(param, None)),
.map(|param| model::ListPart::Item(self.export_type_param(param, None))),
);
let types = self.make_term(model::Term::List { items, tail: None });
let types = self.make_term(model::Term::List { parts });
self.make_term(model::Term::ApplyFull {
global: model::GlobalRef::Named(TERM_PARAM_TUPLE),
args: self.bump.alloc_slice_copy(&[types]),
Expand All @@ -873,54 +872,26 @@ impl<'a> Context<'a> {
}
}

pub fn export_ext_set(&mut self, t: &ExtensionSet) -> model::TermId {
// Extension sets with variables are encoded using a hack: a variable in the
// extension set is represented by converting its index into a string.
// Until we have a better representation for extension sets, we therefore
// need to try and parse each extension as a number to determine if it is
// a variable or an extension.

// NOTE: This overprovisions the capacity since some of the entries of the row
// may be variables. Since we panic when there is more than one variable, this
// may at most waste one slot. That is way better than having to allocate
// a temporary vector.
//
// Also `ExtensionSet` has no way of reporting its size, so we have to count
// the elements by iterating over them...
let capacity = t.iter().count();
let mut extensions = BumpVec::with_capacity_in(capacity, self.bump);
let mut rest = None;

for ext in t.iter() {
if let Ok(index) = ext.parse::<usize>() {
// Extension sets in the model support at most one variable. This is a
// deliberate limitation so that extension sets behave like polymorphic rows.
// The type theory of such rows and how to apply them to model (co)effects
// is well understood.
//
// Extension sets in `hugr-core` at this point have no such restriction.
// However, it appears that so far we never actually use extension sets with
// multiple variables, except for extension sets that are generated through
// property testing.
if rest.is_some() {
// TODO: We won't need this anymore once we have a core representation
// that ensures that extension sets have at most one variable.
panic!("Extension set with multiple variables")
}
pub fn export_ext_set(&mut self, ext_set: &ExtensionSet) -> model::TermId {
let capacity = ext_set.iter().size_hint().0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, so ExtensionSet::iter() just returns an impl Iterator. I wasn't familiar with this size_hint but...are we sure BumpVec::from_iter_in doesn't use it?

Copy link
Contributor

@acl-cqc acl-cqc Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It fact it does (so further to my other comment) - from_iter_in goes to extend which uses the size_hint: https://github.com/fitzgen/bumpalo/blob/8392c63d956962011a127bfd2f98c64aa36ff24a/src/collections/vec.rs#L2149-L2159

Copy link
Contributor Author

Choose a reason for hiding this comment

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

size_hint gives a lower and an optional upper bound for the contents of an iterator. Extension sets do not report their size with the current API, and I didn't want to do a hugr-core API change in this PR.

BumpVec::from_iter_in very likely uses the size_hint. Same reason here for BumpVec::push as in other places: this makes it a bit easier to refactor for now, when everything is still quite in flux.

let mut parts = BumpVec::with_capacity_in(capacity, self.bump);

let node = self.local_scope.expect("local variable out of scope");
rest = Some(
self.module
.insert_term(model::Term::Var(model::LocalRef::Index(node, index as _))),
);
} else {
extensions.push(self.bump.alloc_str(ext) as &str);
for ext in ext_set.iter() {
// `ExtensionSet`s represent variables by extension names that parse to integers.
match ext.parse::<u16>() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This hack from hugr-core is probably worth noting here (sthg like "Variables in extension sets are represented as numbers as these are not valid as Extension names")

Ok(var) => {
let node = self.local_scope.expect("local variable out of scope");
let local_ref = model::LocalRef::Index(node, var);
let term = self.make_term(model::Term::Var(local_ref));
parts.push(model::ExtSetPart::Splice(term));
}
Err(_) => parts.push(model::ExtSetPart::Extension(self.bump.alloc_str(ext))),
}
}

let extensions = extensions.into_bump_slice();

self.make_term(model::Term::ExtSet { extensions, rest })
self.make_term(model::Term::ExtSet {
parts: parts.into_bump_slice(),
})
}

pub fn export_node_metadata(
Expand Down
Loading
Loading