Re-spec and Implement OutlineCfg rewrite #225

acl-cqc · 2023-06-27T17:10:59Z

I re-spec'd the rewrite as it was waay too complex, and the transformations I was doing to handle the more awkward cases can all be done via "replace" and "InsertIdentity" first. However, there's still a case where the region you want to nest includes the CFG Entry node (in which case there would be zero incoming edges, and the new basic block would be the entry node of the existing CFG). The spec rules this out but I don't think there is any workaround. I could implement it in the code but it would make it significantly more complicated (at least a couple of "if"s and a bunch more code). I'm wondering if we should add a special-case version of insertIdentity to add a new node before the entry node of a CFG; thoughts?
- Update: see comment here
I also added a new method to the builder to allow me to create a CFGBuilder object on an existing CFG-node (so I can then use the CFGBuilder to construct a new Block and then a new CFG inside that).
Tieing this all together with the nest_cfgs.rs analysis will be the next PR. At the least, I'll need a way to apply rewrites to a HugrView/HugrMut/subregion-of-a-Hugr...

…r::create_with_io

…o ensure this.

…neighbours

acl-cqc · 2023-07-03T15:15:36Z

src/algorithm/nest_cfgs.rs

@@ -513,23 +513,14 @@ pub(crate) mod test {
        //  entry -> head -> split            > merge -> tail -> exit
        //             |          \-> right -/             |
        //             \---<---<---<---<---<---<---<---<---/
+        // split is unique successor of head


This was a driveby, but I'm doing the same in the new code. (It seemed small enough just to duplicate.)

src/hugr/rewrite/simple_replace.rs

cqc-alec · 2023-07-04T10:32:17Z

However, there's still a case where the region you want to nest includes the CFG Entry node (in which case there would be zero incoming edges, and the new basic block would be the entry node of the existing CFG). The spec rules this out but I don't think there is any workaround. I could implement it in the code but it would make it significantly more complicated (at least a couple of "if"s and a bunch more code). I'm wondering if we should add a special-case version of insertIdentity to add a new node before the entry node of a CFG; thoughts?

Maybe this is the excuse we need to make the CFG's entry node special, exactly mirroring the exit node -- so it would not be a DFB, but just have outputs and no DFG inside it.

At the cost of introducing an extra node, if it makes the replacement API simpler I think it may be worth it. Thoughts?

specification/hugr.md

cqc-alec · 2023-07-04T10:59:28Z

src/builder/cfg.rs

@@ -95,6 +97,20 @@ impl<B: AsMut<Hugr> + AsRef<Hugr>> CFGBuilder<B> {
            inputs: Some(input),
        })
    }
+    pub(crate) fn from_existing(base: B, cfg_node: Node) -> Result<Self, BuildError> {


Should probably add a docstring for this?

I must be missing something here but I don't see how we get away with throwing away the entry node...

Is there a test for this method?

It's not pub - but yes, docstring added.

I've clued about the entry node (the thing is that creating a CFGBuilder doesn't create an entry node - you have to do that explicitly later using e.g. simple_entry_builder and it will prepend that onto the list of children rather than append).

And added a test. (That creates a disconnected basic block, but it'll do for the test.)

src/hugr/rewrite/outline_cfg.rs

cqc-alec · 2023-07-04T11:52:47Z

src/hugr/rewrite/outline_cfg.rs

+}
+
+impl Rewrite for OutlineCfg {
+    type Error = OutlineCfgError;


Why this, is it used?

It's required for the Rewrite trait, as the type of errors that can be returned by the apply method

cqc-alec · 2023-07-04T11:56:07Z

src/hugr/rewrite/outline_cfg.rs

+
+impl Rewrite for OutlineCfg {
+    type Error = OutlineCfgError;
+    const UNCHANGED_ON_FAILURE: bool = true;


Is this used?

again, required for Rewrite trait. (If you put anything in the impl <Trait> for .... block that isn't part of the Trait, you'll get an error. Methods "private" to OutlineCfg would be in impl OutlineCfg i.e. not for any trait.)

src/hugr/rewrite/outline_cfg.rs

acl-cqc · 2023-07-04T13:21:22Z

However, there's still a case where the region you want to nest includes the CFG Entry node (in which case there would be zero incoming edges, and the new basic block would be the entry node of the existing CFG). The spec rules this out but I don't think there is any workaround. I could implement it in the code but it would make it significantly more complicated (at least a couple of "if"s and a bunch more code). I'm wondering if we should add a special-case version of insertIdentity to add a new node before the entry node of a CFG; thoughts?

Maybe this is the excuse we need to make the CFG's entry node special, exactly mirroring the exit node -- so it would not be a DFB, but just have outputs and no DFG inside it.

At the cost of introducing an extra node, if it makes the replacement API simpler I think it may be worth it. Thoughts?

Yes, I had been wondering something similar. (Actually I'd been wishing we'd done that thing whereby Entry/Exit, and Input/Output for DFGs, were edges to/from the parent/container node, but never mind ;-) ). There are some possibilities:

Require the entry node to have no predecessors. In which case there's no need to have any children, either (they could go outside the CFG and their outputs wired as inputs to the CFG). So...
Add a new "Entry block" subtype (BasicBlock::Entry - we have BasicBlock::DFB and BasicBlock::Exit currently). I guess this has no inputs (implicitly these are the same as the CFG's inputs, just as the outputs of a BasicBlock::Exit are the same as the CFG's outputs), but the same predicate_variants and other_outputs as a normal DFB. [Note the Entry node could include a branch. If it doesn't branch, then really its single successor should have other predecessors, or else we could normalize by merging basic blocks.]
Combine the proposed BasicBlock::Entry with BasicBlock::Exit into a BasicBlock::Identity and use that for both entry and exit. I guess this would have predicate_variants and other_outputs, with the idea that
- for the entry block, the CFG inputs are equal to the Sum of the predicate_variants, plus the other outputs
- for the exit block, the predicate_variants are the empty list (this is consistent with it having no successors) - that is, a 0-ary Sum, a type of which there are no instances. The inputs and other_outputs are then identical with the outputs of the CFG.

The latter seems both the most sensible and also a bit weird. Hmmm, I'll keep thinking.

I was also wondering if maybe we should just allow the set of blocks to include the entry block. Since the entry block may have other predecessors too, it may still share most of the code with the standard case (where entry block is not in the set of blocks) - I think I will code this up first and see how it looks.

cqc-alec · 2023-07-04T14:18:35Z

However, there's still a case where the region you want to nest includes the CFG Entry node (in which case there would be zero incoming edges, and the new basic block would be the entry node of the existing CFG). The spec rules this out but I don't think there is any workaround. I could implement it in the code but it would make it significantly more complicated (at least a couple of "if"s and a bunch more code). I'm wondering if we should add a special-case version of insertIdentity to add a new node before the entry node of a CFG; thoughts?

Maybe this is the excuse we need to make the CFG's entry node special, exactly mirroring the exit node -- so it would not be a DFB, but just have outputs and no DFG inside it.
At the cost of introducing an extra node, if it makes the replacement API simpler I think it may be worth it. Thoughts?

Yes, I had been wondering something similar. (Actually I'd been wishing we'd done that thing whereby Entry/Exit, and Input/Output for DFGs, were edges to/from the parent/container node, but never mind ;-) ). There are some possibilities:
* Require the entry node to have no predecessors. In which case there's no need to have any children, either (they could go outside the CFG and their outputs wired as inputs to the CFG). So...

* Add a new "Entry block" subtype (BasicBlock::Entry - we have BasicBlock::DFB and BasicBlock::Exit currently). I guess this has no inputs (implicitly these are the same as the CFG's inputs, just as the outputs of a BasicBlock::Exit are the same as the CFG's outputs), but the same `predicate_variants` and `other_outputs` as a normal DFB. [Note the Entry node could include a branch. If it doesn't branch, then really its single successor should have other predecessors, or else we could normalize by merging basic blocks.]

* Combine the proposed `BasicBlock::Entry` with `BasicBlock::Exit` into a `BasicBlock::Identity` and use that for both entry and exit. I guess this would have `predicate_variants` and `other_outputs`, with the idea that
  
  * for the entry block, the CFG inputs are equal to the Sum of the predicate_variants, plus the other outputs
  * for the exit block, the predicate_variants are the empty list (this is consistent with it having no successors) - that is, a 0-ary Sum, a type of which there are no instances. The inputs and other_outputs are then identical with the outputs of the CFG.
The latter seems both the most sensible and also a bit weird. Hmmm, I'll keep thinking.

I was also wondering if maybe we should just allow the set of blocks to include the entry block. Since the entry block may have other predecessors too, it may still share most of the code with the standard case (where entry block is not in the set of blocks) - I think I will code this up first and see how it looks.

I'm thinking some sort of hybrid of these ideas:

Have distinct node types BasicBlock::{Entry,Exit,DFB}. I think using the same node type for both entry and exit is a little confusing since the conditions on them are different, although operationally they are both the identity.
Neither Entry nor Exit has children.
Require the Entry node to have no predecessors, no inputs and a single successor; the inputs to the DFG in that successor are just the inputs to the CFG. (We have a choice between doing the logic for the first branch in a DFB node or in a node preceding the CFG; here I choose the first option since it seems a bit less weird.)

…g op_types" This reverts commit 47cdc3f.

acl-cqc · 2023-07-04T15:55:12Z

Ok, I've gone with allowing the set to include the entry node (because it is, after all, just another DFB). You can see the changes necessary in this commit: ddf8fb6 - not that hard at all. Beyond that I've added a test of moving the entry node, refactoring some other test code in nest_cfgs.rs.

(Contrastingly allowing the exit block is much harder, somewhat because it's a separate block type, also the second node in children not the first, and if we move the exit block into the sub-cfg, we need a new exit block because the sub-cfg cannot be the exit block - whereas it can be the entry block.)

I wonder if we need a testutils file/class, maybe even a test_cfgs library - nest_cfgs.rs (in src/algorithm) is a long way away from src/hugr/rewrite/outline_cfg.rs...

acl-cqc · 2023-07-04T15:56:13Z

If we want to add another BasicBlock type, etc., I think that's a wider change, and best in another PR - we could do that after this, but I don't think this is so bad that we have to do that first (and quite possibly not at all)

cqc-alec

Looks good, just a few textual suggestions.

specification/hugr.md

acl-cqc added 28 commits June 25, 2023 10:23

simple_replace.rs: use HugrMut::remove_node, includes clearing op_types

47cdc3f

WIP add outline_cfg.rs

f470736

Simplify spec

b40aae4

Outline-cfg compiles, non-exit-block-moving version; expose DFGBuilde…

9d4b261

…r::create_with_io

Common-up connecting new_block's successor

610f1c7

Rename cfg_outputs => outputs

e0f75c6

sub_cfg(_XXX) => cfg(_XXX), also _node.index => _index

8f9b7c4

fmt, remove obsolete comment

7981793

Compute outputs by first matching on cfg_succ

e8460dc

Rename cfg_succ => exit_succ

57a35a5

Try moving the exit_block into the inner CFG. Not actually easier...

0fd40c9

Simplify: always require an exitting edge. InsertIdentity is enough t…

4ae2fc9

…o ensure this.

clippy

59c2ce7

Use matches!

010061f

Use at_most_one rather than manual loop

3528a65

ws

9de20df

Merge remote-tracking branch 'origin/main' into HEAD

00092ad

Merge remote-tracking branch 'origin/main' into new/outline_cfg

ff2d0ac

CFGBuilder: add from_existing

a7940f3

Re-hide create_with_io; use CFGBuilder::from_existing

e9c3c12

Fix entry/exit of CFG node

df4f1d7

nest_cfgs: update test, easiest to get split/merge using (in/out)put_…

85a05ad

…neighbours

Add first test of outline_cfg working!

e90de4d

Specify set of blocks not entry/exit

7572924

And update spec, noting deficiency

6b38017

Further clarify spec

eea616c

clippy

cc44630

Merge remote-tracking branch 'origin/main' into new/outline_cfg

d64c3e0

acl-cqc commented Jul 3, 2023

View reviewed changes

src/hugr/rewrite/simple_replace.rs Outdated Show resolved Hide resolved

cqc-alec requested changes Jul 4, 2023

View reviewed changes

[Refactor] Rename external => external_succs

6cbba93

acl-cqc added 9 commits July 4, 2023 15:18

Region exit node is NOT the CFG exit, so simplify signature computation

5f4062b

Whitespace

4512bf7

Allow blocks to include CFG entry

ddf8fb6

nest_cfgs::test: Break out build_cond_then_loop_cfg

381dc80

build_cond_then_loop: add separate param (use false)

27463ae

Test moving entry

f0d36c1

Merge remote-tracking branch 'origin/main' into new/outline_cfg

d7e9ad9

Refine spec to allow entry node in/out of set, but exclude exit node

1588016

Revert "simple_replace.rs: use HugrMut::remove_node, includes clearin…

86e3fe2

…g op_types" This reverts commit 47cdc3f.

acl-cqc mentioned this pull request Jul 4, 2023

[trivial] simple_replace.rs: use HugrMut::remove_node, includes clearing op_types #242

Merged

acl-cqc added 6 commits July 4, 2023 17:21

exitting -> exiting

dad6c5f

[spec] Remove bogus note on inserting identity before entry block

382e70c

an -> a for consistency

369f62e

Comment and test from_existing

e5f83e6

Make OutlineCfg a struct with blocks

6eca8a7

Merge remote-tracking branch 'origin/main' into new/outline_cfg

a4d02d9

acl-cqc requested a review from cqc-alec July 4, 2023 18:25

cqc-alec approved these changes Jul 5, 2023

View reviewed changes

specification/hugr.md Outdated Show resolved Hide resolved

specification/hugr.md Outdated Show resolved Hide resolved

specification/hugr.md Outdated Show resolved Hide resolved

specification/hugr.md Outdated Show resolved Hide resolved

specification/hugr.md Outdated Show resolved Hide resolved

acl-cqc added 2 commits July 5, 2023 10:08

Spec wording tweaks

37b2b7a

And some more.

8d0d0b1

acl-cqc merged commit 0e6fcc7 into main Jul 5, 2023

acl-cqc deleted the new/outline_cfg branch July 5, 2023 09:58

acl-cqc mentioned this pull request Jul 5, 2023

Transform CFGs into nested form #247

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-spec and Implement OutlineCfg rewrite #225

Re-spec and Implement OutlineCfg rewrite #225

acl-cqc commented Jun 27, 2023 •

edited

Loading

acl-cqc Jul 3, 2023

cqc-alec commented Jul 4, 2023

cqc-alec Jul 4, 2023

acl-cqc Jul 4, 2023

cqc-alec Jul 4, 2023

acl-cqc Jul 4, 2023

cqc-alec Jul 4, 2023

acl-cqc Jul 4, 2023

acl-cqc commented Jul 4, 2023

cqc-alec commented Jul 4, 2023 •

edited

Loading

acl-cqc commented Jul 4, 2023

acl-cqc commented Jul 4, 2023

cqc-alec left a comment

Re-spec and Implement OutlineCfg rewrite #225

Re-spec and Implement OutlineCfg rewrite #225

Conversation

acl-cqc commented Jun 27, 2023 • edited Loading

acl-cqc Jul 3, 2023

Choose a reason for hiding this comment

cqc-alec commented Jul 4, 2023

cqc-alec Jul 4, 2023

Choose a reason for hiding this comment

acl-cqc Jul 4, 2023

Choose a reason for hiding this comment

cqc-alec Jul 4, 2023

Choose a reason for hiding this comment

acl-cqc Jul 4, 2023

Choose a reason for hiding this comment

cqc-alec Jul 4, 2023

Choose a reason for hiding this comment

acl-cqc Jul 4, 2023

Choose a reason for hiding this comment

acl-cqc commented Jul 4, 2023

cqc-alec commented Jul 4, 2023 • edited Loading

acl-cqc commented Jul 4, 2023

acl-cqc commented Jul 4, 2023

cqc-alec left a comment

Choose a reason for hiding this comment

acl-cqc commented Jun 27, 2023 •

edited

Loading

cqc-alec commented Jul 4, 2023 •

edited

Loading