feat: Compile lists and comprehensions #70

mark-koch · 2023-12-20T13:48:50Z

Compiles list comprehensions into a Hugr via nested Loops and Conditionals

acl-cqc

Generally good-to-great. This is quite complex stuff and I don't have any magic solutions to make things simpler! (Tho one suggestion about combining ifs might help a bit. Possibly.) There are some suggestions for wider-scoped followups, but mostly these are quite small.

One could add a ConditionalCompiler, or something like that, that knows that inside the conditional you only have Cases and does not have a self.dfg (because the body of the conditional isn't a Dfg)...I'm not sure that helps enough to justify the extra code, however.

But, is there a different approach? I find DesugaredConditional a bit odd - it does a lot of temporary-variable generation in advance and the two-stage approach seems a little hard to follow. Maybe that could be improved by minor tweaks, but just to outline an alternative:

Replace DesugaredConditional. Rather than a list of generators each with a list of if's, use a nested structure: each (generator or guard) contains the next (generator or guard) as a field, and you recurse on that, using the visitor pattern to choose between generator and guard. At the end of the list you'd have the return element. Then you'd have a ForComprehensionCompiler that only knows about for (generator), if (guard), and on seeing any other expression delegates to ExprCompiler. Then at the start you call FCC with the outer TreeStructuredGenerator.

I think this might be clearer, but I'm not sure enough that it'd be enough of an improvement (if any) to argue very strongly. But maybe have a think :)

guppy/compiler/expr_compiler.py

acl-cqc · 2024-01-10T12:18:58Z

guppy/compiler/expr_compiler.py

+    ) -> Iterator[None]:
+        """Context manager to build a graph inside a new `DFContainer`.
+
+        Automatically updates `self.dfg` and makes the inputs available.


It's unusual to have Iterator[None] and yield nothing (normally you'd yield the object that is here self.dfg), but agreed we have self.dfg already and need it everywhere, so ok.

Proposal for future (not this PR) though - is DFContainer the right name? It has a DFContainingNode, which sounds like much the same thing, and in fact the DFContainer doesn't contain anything (the ...Node) does. Maybe (DFG|DSG)(Compiler|Adaptor|Wrapper) (dataflow sibling graph) to emphasize that it's about (helping with) compiling a single graph/level? Then we could call this _in_subgraph or in_context or similar - the point being that contextmanager here doesn't create the containing node, only some book-keeping necessary for compiling into said containing node.

All naming/semantic nits, though. This all looks pretty good :)

I agree, the naming is not ideal. I added an issue to rethink this #90

acl-cqc · 2024-01-10T12:21:43Z

guppy/compiler/expr_compiler.py

+        old = self.dfg
+        inp = self.graph.add_input(parent=node)
+        new_locals = {
+            name.id: PortVariable(name.id, inp.add_out_port(get_type(name)), name, None)


paranoia, but is it worth checking inputs are all unique? (Does the order matter, could inputs be a set?)

The order does matter (for example since it has to be the same for both Cases).

Adding a uniqueness check sounds good 👍

acl-cqc · 2024-01-10T12:21:57Z

guppy/compiler/expr_compiler.py

+        """
+        loop = self.graph.add_tail_loop([self.visit(name) for name in inputs], parent)
+        with self._new_dfcontainer(inputs, loop):
+            yield


Hahaha! Very cool :)

acl-cqc · 2024-01-10T12:24:03Z

guppy/compiler/expr_compiler.py

+        with self._new_dfcontainer(inputs, loop):
+            yield
+            # Output the branch predicate and the inputs for the next iteration
+            self.graph.add_output(


Makes reasonable sense until you are familiar with Hugr...then I am wondering, how does this get turned into a TupleSum (as the underlying Hugr TailLoop requires)?

Type checking ensures that branch has type bool, which is turned into the Hugr type Sum((), ())

Ah! I see - right, so suddenly this "obviously works" :-). But (IIUC) - in the comment - the "inputs for the next iteration" are the extra values not in the tuple-sum, so they are also loop exit values. Maybe just call them loop variants or something like that?

acl-cqc · 2024-01-10T14:11:42Z

guppy/compiler/expr_compiler.py

+                # generator
+                with self._new_case(inputs, inputs, cond):
+
+                    def compile_ifs(ifs: list[ast.expr]) -> None:


Is there another way here? We already have nested compile_gens in order to get the correct nested/exit behaviour, but how about if there are multiple if's just turning them into a conjunction?

(I was also wondering about := inside such if guards....)

At the moment, short-circuiting expressions and := assignments are not allowed in list comprehensions at all, since we can only compile them as a CFG for now.

Struggling to figure out what you mean here - by "as a CFG" do you mean "as a DFG"? That would make a lot more sense....

ah, the "as a CFG" applies to the "short-circuiting expressions" and ":= assignments", not to the "list comprehensions"....(same thing, I'd read it inverted).

So there's no reason one couldn't pass extra variables defined by := into a Conditional, but this does sound quite hard (i.e. certainly not something we'd want to have to get right more than once!); and you'd have to have nested Conditionals.

However, given a single if guard could itself be a shortcircuit (and/or with :=) then I think making a nested CFG (selecting between the true/false arms here, with perhaps extra variables being introduced along the way) would be the ideal. Happy not to do that in this PR, though - just good to comment/document the limitations

I am also just slightly wondering about how you're handling the variables bound by each for - presumably these are added to the context by an earlier stage in compilation?

acl-cqc · 2024-01-10T14:22:04Z

guppy/compiler/expr_compiler.py

+            yield
+            # Output the branch predicate and the inputs for the next iteration
+            self.graph.add_output(
+                [self.visit(branch), *(self.visit(name) for name in inputs)]


note repeated calls to self.visit(name) for name in inputs from a few lines above. Presume that's ok? Does it make sense to common up?

We could add a helper function

def compile_inputs() -> Iterator[OutPortV]: return self.visit(name) for name in inputs

but not sure if that makes it clearer? The point is that we have to make a fresh call to self.visit since we are in a new nested context

Ah I see. Don't think I'd bother with the helper, but commenting that we need to revisit in the new context would be good

guppy/compiler/expr_compiler.py

guppy/hugr/hugr.py

acl-cqc

I'd hit approve at this point if there were tests...I'm slightly nervous of doing so without so what's the situation there?

mark-koch · 2024-01-12T14:15:16Z

The tests are here: #71

acl-cqc

Looks good, thanks. One last thing is that it sounds like an if guard that just contains an and (or or) will not be compiled - that is a bit of a limitation, we should doc that somewhere. (and you could compile to another if, but not or so I don't think I'd bother)

mark-koch · 2024-01-15T10:22:16Z

That is a bit of a limitation, we should doc that somewhere.

Created issue #98 for this.

and you could compile to another if, but not or so I don't think I'd bother

Agreed

mark-koch added 4 commits December 20, 2023 13:25

feat: Build for loops and comprehensions

359c2da

feat: Add list and comprehension type checking

36396cc

Fix error golden files

000e146

feat: Compile lists and comprehensions

34d5415

mark-koch mentioned this pull request Dec 20, 2023

feat: Add iterators and lists #67

Merged

mark-koch force-pushed the lists/compile branch from da94ee2 to 34d5415 Compare December 20, 2023 13:56

mark-koch requested a review from acl-cqc January 3, 2024 10:28

mark-koch added 5 commits January 8, 2024 12:20

Improve instance method helper function

f51f0bf

Don't override usage stats

01319cc

Include parent scope in Locals.__iter__

2359ab0

Don't require linear return for __next__

81dcddb

Improve error msg

34e9ce2

ss2165 added this to the Release v0.1 milestone Jan 10, 2024

acl-cqc reviewed Jan 10, 2024

View reviewed changes

mark-koch added 4 commits January 11, 2024 10:09

Add comment to visit_List

f8e6768

Check uniqueness of input names

b02dc3d

Rename _new_loop inputs to variants

34ba5e1

Fix spelling and clarify docstring

17aca09

mark-koch mentioned this pull request Jan 11, 2024

Switch arguments of with_type and with_loc #89

Open

mark-koch added 5 commits January 11, 2024 10:40

Fix _new_case inputs for outputs

7639089

Add comment explaining passing inputs twice

8cb0df0

Use inputs comprehension

20b2d99

Fix comments

a14ae45

Add _if_true context manager

8f561e2

mark-koch mentioned this pull request Jan 11, 2024

Rethink naming of DFContainer #90

Open

acl-cqc reviewed Jan 12, 2024

View reviewed changes

acl-cqc approved these changes Jan 15, 2024

View reviewed changes

mark-koch added 2 commits January 15, 2024 10:15

Rename variants to loop_vars

1fb02a0

Add comment explaining fresh visit calls

13eac52

mark-koch added 2 commits January 15, 2024 17:38

Merge branch 'feat/lists' into lists/build

633421d

Add make_assign helper

7630f7b

mark-koch self-assigned this Jan 16, 2024

mark-koch added 6 commits January 16, 2024 12:37

Turn TemplateReplacer into dataclass

70c5d07

Add missing location

fd8f56c

Adjust context in make_assign

6790161

Merge remote-tracking branch 'origin/lists/build' into lists/check

f5e4ea9

Merge remote-tracking branch 'origin/lists/check' into lists/compile

74102c3

Fix formatting

81d1722

Base automatically changed from lists/check to feat/lists January 16, 2024 13:02

mark-koch merged commit 270ea9e into feat/lists Jan 16, 2024
1 check passed

mark-koch deleted the lists/compile branch January 16, 2024 13:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Compile lists and comprehensions #70

feat: Compile lists and comprehensions #70

mark-koch commented Dec 20, 2023 •

edited

Loading

acl-cqc left a comment

acl-cqc Jan 10, 2024 •

edited

Loading

mark-koch Jan 11, 2024

acl-cqc Jan 10, 2024

mark-koch Jan 11, 2024

acl-cqc Jan 10, 2024

acl-cqc Jan 10, 2024

mark-koch Jan 11, 2024

acl-cqc Jan 12, 2024 •

edited

Loading

acl-cqc Jan 10, 2024

acl-cqc Jan 10, 2024

mark-koch Jan 11, 2024

acl-cqc Jan 12, 2024

acl-cqc Jan 12, 2024

acl-cqc Jan 12, 2024

acl-cqc Jan 10, 2024

mark-koch Jan 11, 2024

acl-cqc Jan 12, 2024

acl-cqc left a comment

mark-koch commented Jan 12, 2024

acl-cqc left a comment

mark-koch commented Jan 15, 2024

feat: Compile lists and comprehensions #70

feat: Compile lists and comprehensions #70

Conversation

mark-koch commented Dec 20, 2023 • edited Loading

acl-cqc left a comment

Choose a reason for hiding this comment

acl-cqc Jan 10, 2024 • edited Loading

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

acl-cqc Jan 12, 2024 • edited Loading

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

Choose a reason for hiding this comment

acl-cqc left a comment

Choose a reason for hiding this comment

mark-koch commented Jan 12, 2024

acl-cqc left a comment

Choose a reason for hiding this comment

mark-koch commented Jan 15, 2024

mark-koch commented Dec 20, 2023 •

edited

Loading

acl-cqc Jan 10, 2024 •

edited

Loading

acl-cqc Jan 12, 2024 •

edited

Loading