feat: Lower collections extension #1720

mark-koch · 2024-11-26T17:34:06Z

Closes #1701 and fixes #1645.

Note that destructors and ref-counting hooks are still missing. These will follow in a future PR once they are available in hugr-llvm.

codecov · 2024-11-26T18:32:58Z

Codecov Report

Attention: Patch coverage is 82.88591% with 51 lines in your changes missing coverage. Please review.

Project coverage is 86.35%. Comparing base (fc609a2) to head (31b06a7).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
hugr-llvm/src/extension/collections.rs	83.65%	4 Missing and 39 partials ⚠️
hugr-llvm/src/emit/func.rs	71.42%	0 Missing and 8 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1720      +/-   ##
==========================================
- Coverage   86.38%   86.35%   -0.03%     
==========================================
  Files         166      167       +1     
  Lines       30063    30361     +298     
  Branches    26975    27273     +298     
==========================================
+ Hits        25969    26219     +250     
- Misses       2558     2559       +1     
- Partials     1536     1583      +47

Flag	Coverage Δ
python	`92.42% <ø> (ø)`
rust	`85.67% <82.88%> (-0.02%)`	⬇️

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.

doug-q

Looks great! Some nits, see what you think. I understand execution tests would be hard, could you add an issue for those?

doug-q · 2024-11-27T10:37:09Z

hugr-core/src/std_extensions/collections.rs

@@ -59,6 +59,11 @@ impl ListValue {
    pub fn custom_type(&self) -> CustomType {
        list_custom_type(self.1.clone())
    }
+
+    /// Returns the values contained inside the `[ListValue]`.
+    pub fn get_contents(&self) -> &Vec<Value> {


I think this is more idomatic returning &[Value]. Not too bothered.

doug-q · 2024-11-27T10:37:39Z

hugr-llvm/src/extension/collections.rs

+/// delegates everything to those default implementations.
+pub trait CollectionsCodegen: Clone {
+    /// Return the llvm type of [hugr_core::std_extensions::collections::LIST_TYPENAME].
+    fn list_type<'c>(&self, session: &TypingSession<'c, '_>) -> BasicTypeEnum<'c> {


Suggested change

fn list_type<'c>(&self, session: &TypingSession<'c, '_>) -> BasicTypeEnum<'c> {

fn list_type<'c>(&self, session: TypingSession<'c, '_>) -> BasicTypeEnum<'c> {

doug-q · 2024-11-27T10:39:46Z

hugr-llvm/src/extension/collections.rs

+    }
+
+    /// Helper function to compute the signature of runtime functions.
+    fn rt_func_sig<'c>(


I don't like these bools, I would prefer something like pub enum CollectionRuntimeFunc { ... }. If you do keep the bools please elaborate in the comment what they mean.

doug-q · 2024-11-27T10:42:00Z

hugr-llvm/src/extension/collections.rs

+            ],
+            false,
+        );
+        let func = ctx.get_extern_func("__rt__list__new", func_ty)?;


I understand that this is not the worst. Users with a different function name could reimplement this. I would much prefer self.runtime_func_name(CollectionsRuntimeFunc::new) though.

doug-q · 2024-11-27T10:43:55Z

hugr-llvm/src/extension/collections.rs

+    }
+}
+
+fn build_option<'c, H: HugrView>(


This would be better as a free function in crate::emit::func, there are many other places that would like it.

doug-q · 2024-11-27T10:44:14Z

hugr-llvm/src/extension/collections.rs

+    Ok(option)
+}
+
+fn build_ok_or_else<'c, H: HugrView>(


as for build_option

doug-q · 2024-11-27T10:45:27Z

hugr-llvm/src/extension/collections.rs

+    where
+        Self: 'a,
+    {
+        add_collections_extensions(builder, self.0)


My current thinking is that the free add_collections_extension is unnecessary, we should just put it's implementation here. What do you think?

doug-q · 2024-11-27T10:46:56Z

hugr-llvm/src/extension/collections.rs

+        _ => {
+            return Err(anyhow!("Collections: invalid type args for list op"));


Suggested change

_ => {

return Err(anyhow!("Collections: invalid type args for list op"));

args => bail!("Collections: invalid type args for list op: {args:?}"));

mark-koch · 2024-11-27T14:58:25Z

I added the runtime function enum and removed most of the functions in CollectionsCodegen. Agreed that this is a lot nicer 👍

Also created issue #1722 for execution tests

doug-q

Awesome, thank you Mark.

doug-q · 2024-11-27T15:06:44Z

hugr-llvm/src/extension/collections.rs

+};
+
+/// Runtime functions that implement operations on lists.
+#[derive(Clone, Copy, Debug, PartialEq, Hash)]


Suggested change

#[derive(Clone, Copy, Debug, PartialEq, Hash)]

#[derive(Clone, Copy, Debug, PartialEq, Hash)]

#[non_exhaustive]

would mean adding enums isn't a breaking change

mark-koch requested a review from doug-q November 26, 2024 17:34

mark-koch requested a review from a team as a code owner November 26, 2024 17:34

feat: Lower collections extension

e6f8e1a

mark-koch force-pushed the mk/collections branch from 26d8498 to e6f8e1a Compare November 26, 2024 17:49

doug-q reviewed Nov 27, 2024

View reviewed changes

mark-koch added 7 commits November 27, 2024 13:14

Return slice instead of vector from get_contents

f26ce15

Take typing session by value

03c2b4f

Refactor to use an enum for rt function

4bf3751

Move build_option and build_ok_or_else to emit::func

f2892d2

Remove free add_collections_extension

73e4585

Use bail

472288e

Fix elem ty lookup and refactor build_alloca_i8_ptr

84c6fb3

Merge branch 'main' into mk/collections

6680e53

mark-koch requested a review from doug-q November 27, 2024 14:59

doug-q approved these changes Nov 27, 2024

View reviewed changes

mark-koch added 2 commits November 27, 2024 15:17

Mark CollectionsRtFunc as non_exhaustive

1f159f4

Add one more bail

31b06a7

mark-koch enabled auto-merge November 27, 2024 15:19

mark-koch added this pull request to the merge queue Nov 27, 2024

Merged via the queue into main with commit c87d4f3 Nov 27, 2024
24 checks passed

mark-koch deleted the mk/collections branch November 27, 2024 15:25

hugrbot mentioned this pull request Nov 27, 2024

chore: release #1670

Closed

This was referenced Dec 10, 2024

chore: release #1757

Closed

chore: release #1761

Closed

chore: release #1778

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Lower collections extension #1720

feat: Lower collections extension #1720

mark-koch commented Nov 26, 2024

codecov bot commented Nov 26, 2024 •

edited

Loading

doug-q left a comment

doug-q Nov 27, 2024

doug-q Nov 27, 2024

doug-q Nov 27, 2024

doug-q Nov 27, 2024

doug-q Nov 27, 2024

doug-q Nov 27, 2024

doug-q Nov 27, 2024

doug-q Nov 27, 2024

mark-koch commented Nov 27, 2024

doug-q left a comment

doug-q Nov 27, 2024

	fn list_type<'c>(&self, session: &TypingSession<'c, '_>) -> BasicTypeEnum<'c> {
	fn list_type<'c>(&self, session: TypingSession<'c, '_>) -> BasicTypeEnum<'c> {

		_ => {
		return Err(anyhow!("Collections: invalid type args for list op"));

	_ => {
	return Err(anyhow!("Collections: invalid type args for list op"));
	args => bail!("Collections: invalid type args for list op: {args:?}"));

	#[derive(Clone, Copy, Debug, PartialEq, Hash)]
	#[derive(Clone, Copy, Debug, PartialEq, Hash)]
	#[non_exhaustive]

feat: Lower collections extension #1720

feat: Lower collections extension #1720

Conversation

mark-koch commented Nov 26, 2024

codecov bot commented Nov 26, 2024 • edited Loading

Codecov Report

doug-q left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-koch commented Nov 27, 2024

doug-q left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 26, 2024 •

edited

Loading