DRAFT: feat: Prototype dataflow analysis for static circuit extraction #1664

doug-q · 2024-11-18T09:26:12Z

Based on #1603.

Includes an rough inliner and call graph.

See the new snapshots for current capabilities.

…t Hash)

* DFContext reinstate fn hugr(), drop AsRef requirement (fixes StackOverflow) * test_tail_loop_iterates_twice: use tail_loop_builder_exts, fix from #1332(?) * Fix only-one-DataflowContext asserts using Arc::ptr_eq

…s what was meant

…g len

…text interprets load_constant

… PV+PS

acl-cqc · 2024-11-18T11:04:31Z

hugr-passes/src/static_circuit.rs

+            return;
+        }
+
+        if ins.iter().any(|x| x == &PartialValue::Bottom) {


I think this demonstrates exactly why I put in the test you comment on here !!

Yes, any bottom inputs => all bottom outputs.

That we have to check this at the linked callsite of interpret_leaf_op and here is annoying. Perhaps interpret_leaf_op should only be called when there are no Bottom inputs?

doug-q · 2024-11-18T11:12:15Z

I think the 125 files changed is because I merged main. Annoying.

acl-cqc

Ok... so my overall thought is this is not really a dataflow analysis. What you want is the part of dataflow analysis that does "join" at the exit of every conditional, and indeed on every control-flow merge; and possibly for TailLoops; but the things you want to join are circuits, not any kind of value that's per-qubit. (Because that join/merge logic currently only exists in dataflow analysis, "give a man a hammer and everything looks like a nail", but the other bits of the dataflow framework are really not helping here, and it'd be better to do something else - another datalog, probably, with just the bit you need. If you really want analysis like "only these cases are reachable" we should determine that using DF and then simplify the Hugr with this knowledge.)

Also, you need to do some much-more-complex factoring of classical values so that you can catch "quantum-part-of-circuit same, classical part different" which you clearly want to do. And there is the isomorphism concern....

So, overall, afraid I still think that the best approach is to say flat circuits (no nested control flow) are statically scheduled; and then we transform into that. That lets us deploy a wide range of different optimizations to get there, perhaps including this or something like it, but with flexibility to choose an optimization according to what works on a particular circuit. (I slightly favour a rewriting approach but don't mean to rule this approach out.)

acl-cqc · 2024-11-18T11:10:33Z

hugr-passes/src/static_circuit.rs

+            }
+        } else {
+            assert_eq!(qb_outs.len(), qb_ins.len(), "{e:?}");
+            let mb_in_qbs = qb_ins


I am finding this quite hard to follow and this is the critical part of the algorithm, so if I've got the wrong end of the stick, my conclusions may be....entirely wrong :). A few comments wouldn't hurt....

Thanks for looking, you've certainly identified the roughest area. Very much WIP.

hugr-passes/src/static_circuit.rs

acl-cqc · 2024-11-18T11:12:35Z

hugr-passes/src/static_circuit.rs

+                .collect::<Option<HashMap<_, _>>>();
+            let gating_qbs = mb_in_qbs
+                .iter()
+                .flat_map(|hash_map| hash_map.iter().map(|(_, qbh)| qbh.qb_index()))


ooof. So is that equivalent to
mb_in_qbs.unwrap_or_default().values().map(QbHistory::qb_index) ?

So the "gating_nodes" of a new node is the ordered list of input gates, right? That could go in a comment on Gate::gating_nodes.

I'm slightly concerned that Node (rather than (Node, Port)) is sufficiently accurate - the wire coming from one or another port sounds like an important difference - I think the counterargument is that if a different port were used, because of linearity other places would see different Node instead, so we might be OK.

acl-cqc · 2024-11-18T11:25:30Z

hugr-passes/src/static_circuit.rs

+                .collect_vec();
+            let qb_out_to_in: HashMap<_, _> = zip_eq(qb_outs, qb_ins).collect();
+            for (i, v) in outs.iter_mut().enumerate() {
+                let mb_join_val = (|| {


The no-args-closure-that-you-immediately-call is so that you can use ? within it to exit just the closure, right?

Consider using continue in the successful case, and then join-ing with Top if you haven't continued

Or even

let mut mb_join_val = PartialValue::Top; if .... { if .... { .... mb_join_val = qbh.into(); } } v.join(mb_join_val);

acl-cqc · 2024-11-18T11:26:19Z

hugr-passes/src/static_circuit.rs

+            for (i, v) in outs.iter_mut().enumerate() {
+                let mb_join_val = (|| {
+                    let in_p = qb_out_to_in.get(&OutgoingPort::from(i))?;
+                    let hash_map = mb_in_qbs.as_ref()?;


Note that if you did unwrap_or_default when constructing mb_in_qbs, so you had an empty map rather than None, then the get(...)? on the line below would do both ?s in one go

acl-cqc · 2024-11-18T12:03:23Z

hugr-passes/src/static_circuit.rs

+                    Some(qbh.into())
+                })();
+
+                v.join_mut(mb_join_val.unwrap_or(PartialValue::Top));


Short comment - if the corresponding in-port has a QbHistory, push this gate (+ ordered list of its inputs) onto that QbHistory...

But, if this in-port is classical, join with Top. Doesn't this hit 3.py where you have two different classical values?

The intent is to always propagate TOP for classical outputs. In StaticCircuitPass we only look at linear QB_T wires, so I don't think it matters that classical wires are TOP.

How does one proceed in that case (given a TOP)? Is the goal here just to identify cases which could be statically scheduled if we then did a separate transformation pass?

TOP only goes on the classical wires. If the circuit structure depends on those top values then we will fail to extract a static circuit.

Once we've extracted a static circuit, we should schedule it, then insert it at the "end" of the hugr(whatever that means). We'll need to wire up the angles. Then delete the original gate ops.

Does this answer your question?

Yes, "then wire up the angles" is the "separate transformation" i.e. where you add extra edges, Output node ports, etc. to get the angles from wherever they were computed.

hugr-passes/src/static_circuit.rs

doug-q · 2024-11-18T14:11:12Z

Ok... so my overall thought is this is not really a dataflow analysis. What you want is the part of dataflow analysis that does "join" at the exit of every conditional, and indeed on every control-flow merge; and possibly for TailLoops; but the things you want to join are circuits, not any kind of value that's per-qubit. (Because that join/merge logic currently only exists in dataflow analysis, "give a man a hammer and everything looks like a nail", but the other bits of the dataflow framework are really not helping here, and it'd be better to do something else - another datalog, probably, with just the bit you need. If you really want analysis like "only these cases are reachable" we should determine that using DF and then simplify the Hugr with this knowledge.)

I disagree. What is your criteria for being a dataflow analysis? Mine is: A well defined join and transfer function.

I have pushed and the test cases are now working. I had to add handling for "arguments of public functions are top". I do not claim that my interpret_leaf_op is understandable yet! (but it seems mostly correct)

acl-cqc · 2024-11-18T15:02:36Z

What is your criteria for being a dataflow analysis?

An abstract value per wire (perhaps only a subset of wires, i.e. only those of type Qubit). Here you want an abstract value per circuit (per sibling subgraph).

I think the analysis framework you want is:

client provides a type of values V. Yes, these will need join.
client provides a function interpret_dfg(parent: Node, child_results: Map<Node, V>) -> V which is called with a map whose keys are the container nodes in the dfg.
- For "nested DFG" nodes, the value in the map is the result of calling interpret_dfg, so we could rely on interpret_dfg to recursively call itself, but I think this just makes the rules more complicated
- The framework handles Conditional nodes by recursing on each child Case, and then joining the resulting V's
- The framework handles CFG nodes similarly by merging at CF joins and copying at control-flow splits. (There is some value in doing Tag/Value/Sum analysis on wires here but I think that could be done separately as a first step)
- The framework handles TailLoop's...somehow (I mean, any quantum op in a tailloop will report "no static circuit" unless it's a never-iterates loop, right?)

The use-case here then instantiates V == Circuit == Hugr<RootNode=DFG>, and interpret_dfg inlines the map (replacing each container node keyed in the Map with the DFG value from the Map)

I think this is necessary to handle qalloc/qfree which are not represented in the outputs (where you suggest "this might require Dense Analysis")

acl-cqc · 2024-11-18T15:09:39Z

Ok, so qalloc/qfree could be represented in the containing region's outputs mentioning (in their gating_nodes) 2-qubit ops with the ancillae. But then you've got only a nodeindex, not a description of the op (and the history of the ancilla), so if you had a conditional which in each arm did qalloc, ..., qfree with (case 1) the same, or (case 2) different ops, to combine the ancilla with the conditional's input-and-output qubit, we still need to do some graph traversal (not here) to identify or distinguish the two arms?

acl-cqc · 2024-11-19T09:48:25Z

Data flows along wires, hence dataflow analysis attaches values (modelling data) to those wires. In order to split a circuit into per-wire "Vertical slices" we have to break up the definitive model of a circuit (i.e. a Hugr) into some per-wire variant. Your QbHistory, with the qubit indices for each gate, is quite a detailed model (given linearity) and as such goes further than I'd anticipated.

qalloc is currently still a problem - replace qalloc() with if p {qalloc()} else {qalloc()} and we'll fail to extract a static circuit. This is soluble by building a yet-more-detailed model of the circuit.
qfree is harder to solve, as there is no output wire from the circuit to which to attach a value describing what was freed. I haven't got a good answer to how to deal with this. Consider fn circuit(q0) { let temp = qalloc() in if p {h(temp); cx(q0, temp); qfree(temp)} else {rz(temp); ry(temp); cx(q0, temp); qfree(temp);} }

One thing I like about the 'rewriting' approach is that it combines analysis with transformation, and I think generalizes fairly straightforwardly to handle both of those cases. But any analysis is likely to have shortcomings, which is why I prefer an input/ready-flattened format that is agnostic to analysis technique. That doesn't rule out the analysis you have here!

doug-q · 2024-11-19T10:29:19Z

Data flows along wires, hence dataflow analysis attaches values (modelling data) to those wires. In order to split a circuit into per-wire "Vertical slices" we have to break up the definitive model of a circuit (i.e. a Hugr) into some per-wire variant. Your QbHistory, with the qubit indices for each gate, is quite a detailed model (given linearity) and as such goes further than I'd anticipated.

qalloc is currently still a problem - replace qalloc() with if p {qalloc()} else {qalloc()} and we'll fail to extract a static circuit. This is soluble by building a yet-more-detailed model of the circuit.

qfree is harder to solve, as there is no output wire from the circuit to which to attach a value describing what was freed. I haven't got a good answer to how to deal with this. Consider fn circuit(q0) { let temp = qalloc() in if p {h(temp); cx(q0, temp); qfree(temp)} else {rz(temp); ry(temp); cx(q0, temp); qfree(temp);} }

One thing I like about the 'rewriting' approach is that it combines analysis with transformation, and I think generalizes fairly straightforwardly to handle both of those cases. But any analysis is likely to have shortcomings, which is why I prefer an input/ready-flattened format that is agnostic to analysis technique. That doesn't rule out the analysis you have here!

I agree that qalloc and qfree are problematic, and that this approach only works when they are simply arranged with all qallocs at the beginning and all the qfrees at the end. I don't think this is unreasonable for extracting a static circuit, but perhaps I'm wrong. The reason I have inlining here is so that I get all the qallocs into main. The raw guppy calls functions that call call qalloc, which defeats the analysis for the same reasons as you describe above.

We are surely in agreement that we should do transformations on the hugr first to make the analysis work better.

acl-cqc · 2024-11-19T11:14:09Z

Yeah ok so I think the things I would disagree with are not actually in this PR - they are the architecture, what analysis/transformation goes where in the pipeline, what the intermediates exchanged look like.

That you can use dataflow analysis to extract (some) static circuits, I do not dispute :-) :-). Whether it's the best tool for the job - well that depends what else is in the toolbox, and right now the toolbox doesn't have a lot else in it and dataflow analysis is the most powerful tool in there.

We are surely in agreement that we should do transformations on the hugr first to make the analysis work better.

Generally I would follow the school of thought that analysis enables transformation, rather than the other way around...

hugrbot · 2024-12-11T10:40:54Z

Hey there and thank you for opening this pull request! 👋

We require pull request titles to follow the Conventional Commits specification
and it looks like your proposed title needs to be adjusted.

Your title should look like this. The scope field is optional.

<type>(<scope>): <description>

If the PR includes a breaking change, mark it with an exclamation mark:

<type>!: <description>

and include a "BREAKING CHANGE:" footer in the body of the pull request.

Details:

Unknown release type "DRAFT" found in pull request title "DRAFT: feat: Prototype dataflow analysis for static circuit extraction". 

Available types:
 - feat
 - fix
 - docs
 - style
 - refactor
 - perf
 - test
 - ci
 - chore
 - revert

acl-cqc added 30 commits September 11, 2024 16:15

Just const_fold2 + inside that partial_value (taken from hugr_core)

c7a9d89

merge/update+fmt (ValueName for ConstInt non-compiling as ConstInt no…

ac45e53

…t Hash)

Missing imports / lints. Now running, but failing w/StackOverflow

8adaa6e

Fix tests...

098c735

* DFContext reinstate fn hugr(), drop AsRef requirement (fixes StackOverflow) * test_tail_loop_iterates_twice: use tail_loop_builder_exts, fix from #1332(?) * Fix only-one-DataflowContext asserts using Arc::ptr_eq

ValueKey using MaybeHash

706c892

tag() does not refer to self.is_compound

63bc944

ValueHandle::{is_compound,num_fields,index} => {variant_values, as_sum}

5fa7edb

Rm ValueHandle::tag, use variant_values - inefficient, presume this i…

98bf94a

…s what was meant

add variant_values, rewrite one use of outputs_for_variant

295ec32

...and the other two; remove outputs_for_variant

5c8289e

Rewrite tuple rule to avoid indexing

bf173ab

GC unused (tuple,variant)_field_value, iter_with_ports

0ae4d19

Common up via ValueRow.unpack_first

8608ba9

No DeRef for ValueHandle, just add get_type()

2dca3e9

ValueKey::{Select->Field,index->field}

51e68ea

(join/meet)_mut_unsafe => try_(join/meet)_mut with Err for conflictin…

8635474

…g len

Remove ValueHandle::variant_values - just have as_sum

80d5b86

Optimize as_sum() by returning impl Iterator not Vec

1c8be99

Machine uses PV not PartialValue

b0afa54

Parametrize PartialValue+PV+Machine by AbstractValue/Into<Value>, Con…

d09a1fe

…text interprets load_constant

Move partial_value.rs inside datalog/

af8827b

Hide PartialSum/PartialValue

4b61436

refactor: ValueRow::single_among_bottoms

8b31d8c

Factor out propagate_leaf_op; add ValueRow::from_iter

6a72961

Add handling for Tag

780af9b

Remove PV (use typedef in datalog.rs)

cabcf04

Allow DFContext to interpret any leaf op (except MakeTuple/etc.); pub…

5e4a04f

… PV+PS

Also fold extension ops

1453886

Comment as_sum

221e96c

Rename DataflowContext to HugrValueContext

cd4e15c

acl-cqc and others added 18 commits November 5, 2024 10:58

refactor: assert_fully_folded takes HugrView

d90190b

refactor: find_needed_nodes works out root itself

8f89c88

CFG test (failing atm)

a6bb507

fix/change-policy: don't break edges from root input

3e05639

Fix: don't follow CFG edges, but don't panic either

5b2a2c9

clippy, moving might_diverge outside ConstFoldPass+parametrize

dcf76a6

Improve const_fold module doc

92488fe

wip

45a9869

wip

d1172c7

Merge remote-tracking branch 'origin/main' into doug/static-circuit

07efbb4

wip

0b36617

wip

c60e2c7

wip

19589b0

wip

f6a6b65

wip

ae857a1

wip

9520633

wip

4a88d90

fmt

35a7c6f

acl-cqc reviewed Nov 18, 2024

View reviewed changes

works

4f15ffa

Base automatically changed from acl/const_fold3 to main December 11, 2024 10:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRAFT: feat: Prototype dataflow analysis for static circuit extraction #1664

DRAFT: feat: Prototype dataflow analysis for static circuit extraction #1664

doug-q commented Nov 18, 2024

acl-cqc Nov 18, 2024

doug-q Nov 18, 2024

doug-q commented Nov 18, 2024

acl-cqc left a comment

acl-cqc Nov 18, 2024

doug-q Nov 18, 2024

acl-cqc Nov 18, 2024

acl-cqc Nov 18, 2024

acl-cqc Nov 18, 2024

acl-cqc Nov 18, 2024

acl-cqc Nov 18, 2024

acl-cqc Nov 18, 2024

doug-q Nov 18, 2024

acl-cqc Nov 18, 2024

doug-q Nov 18, 2024

acl-cqc Nov 18, 2024

doug-q commented Nov 18, 2024

acl-cqc commented Nov 18, 2024

acl-cqc commented Nov 18, 2024 •

edited

Loading

acl-cqc commented Nov 19, 2024 •

edited

Loading

doug-q commented Nov 19, 2024

acl-cqc commented Nov 19, 2024

hugrbot commented Dec 11, 2024

DRAFT: feat: Prototype dataflow analysis for static circuit extraction #1664

Are you sure you want to change the base?

DRAFT: feat: Prototype dataflow analysis for static circuit extraction #1664

Conversation

doug-q commented Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doug-q commented Nov 18, 2024

acl-cqc 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

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

doug-q commented Nov 18, 2024

acl-cqc commented Nov 18, 2024

acl-cqc commented Nov 18, 2024 • edited Loading

acl-cqc commented Nov 19, 2024 • edited Loading

doug-q commented Nov 19, 2024

acl-cqc commented Nov 19, 2024

hugrbot commented Dec 11, 2024

acl-cqc commented Nov 18, 2024 •

edited

Loading

acl-cqc commented Nov 19, 2024 •

edited

Loading