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.

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

Re-spec and Implement OutlineCfg rewrite #225

Merged
merged 46 commits into from
Jul 5, 2023
Merged
Show file tree
Hide file tree
Changes from 44 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
47cdc3f
simple_replace.rs: use HugrMut::remove_node, includes clearing op_types
acl-cqc Jun 25, 2023
f470736
WIP add outline_cfg.rs
acl-cqc Jun 23, 2023
b40aae4
Simplify spec
acl-cqc Jun 23, 2023
9d4b261
Outline-cfg compiles, non-exit-block-moving version; expose DFGBuilde…
acl-cqc Jun 25, 2023
610f1c7
Common-up connecting new_block's successor
acl-cqc Jun 25, 2023
e0f75c6
Rename cfg_outputs => outputs
acl-cqc Jun 25, 2023
8f9b7c4
sub_cfg(_XXX) => cfg(_XXX), also _node.index => _index
acl-cqc Jun 25, 2023
7981793
fmt, remove obsolete comment
acl-cqc Jun 25, 2023
e8460dc
Compute outputs by first matching on cfg_succ
acl-cqc Jun 25, 2023
57a35a5
Rename cfg_succ => exit_succ
acl-cqc Jun 25, 2023
0fd40c9
Try moving the exit_block into the inner CFG. Not actually easier...
acl-cqc Jun 25, 2023
4ae2fc9
Simplify: always require an exitting edge. InsertIdentity is enough t…
acl-cqc Jun 25, 2023
59c2ce7
clippy
acl-cqc Jun 25, 2023
010061f
Use matches!
acl-cqc Jun 25, 2023
3528a65
Use at_most_one rather than manual loop
acl-cqc Jun 25, 2023
9de20df
ws
acl-cqc Jun 25, 2023
00092ad
Merge remote-tracking branch 'origin/main' into HEAD
acl-cqc Jun 30, 2023
ff2d0ac
Merge remote-tracking branch 'origin/main' into new/outline_cfg
acl-cqc Jul 3, 2023
a7940f3
CFGBuilder: add from_existing
acl-cqc Jul 3, 2023
e9c3c12
Re-hide create_with_io; use CFGBuilder::from_existing
acl-cqc Jul 3, 2023
df4f1d7
Fix entry/exit of CFG node
acl-cqc Jul 3, 2023
85a05ad
nest_cfgs: update test, easiest to get split/merge using (in/out)put_…
acl-cqc Jul 3, 2023
e90de4d
Add first test of outline_cfg working!
acl-cqc Jul 3, 2023
7572924
Specify set of blocks not entry/exit
acl-cqc Jul 3, 2023
6b38017
And update spec, noting deficiency
acl-cqc Jul 3, 2023
eea616c
Further clarify spec
acl-cqc Jul 3, 2023
cc44630
clippy
acl-cqc Jul 3, 2023
d64c3e0
Merge remote-tracking branch 'origin/main' into new/outline_cfg
acl-cqc Jul 3, 2023
6cbba93
[Refactor] Rename external => external_succs
acl-cqc Jul 4, 2023
5f4062b
Region exit node is NOT the CFG exit, so simplify signature computation
acl-cqc Jul 4, 2023
4512bf7
Whitespace
acl-cqc Jul 4, 2023
ddf8fb6
Allow blocks to include CFG entry
acl-cqc Jul 4, 2023
381dc80
nest_cfgs::test: Break out build_cond_then_loop_cfg
acl-cqc Jul 4, 2023
27463ae
build_cond_then_loop: add separate param (use false)
acl-cqc Jul 4, 2023
f0d36c1
Test moving entry
acl-cqc Jul 4, 2023
d7e9ad9
Merge remote-tracking branch 'origin/main' into new/outline_cfg
acl-cqc Jul 4, 2023
1588016
Refine spec to allow entry node in/out of set, but exclude exit node
acl-cqc Jul 4, 2023
86e3fe2
Revert "simple_replace.rs: use HugrMut::remove_node, includes clearin…
acl-cqc Jul 4, 2023
dad6c5f
exitting -> exiting
acl-cqc Jul 4, 2023
382e70c
[spec] Remove bogus note on inserting identity before entry block
acl-cqc Jul 4, 2023
369f62e
an -> a for consistency
acl-cqc Jul 4, 2023
e5f83e6
Comment and test from_existing
acl-cqc Jul 4, 2023
6eca8a7
Make OutlineCfg a struct with blocks
acl-cqc Jul 4, 2023
a4d02d9
Merge remote-tracking branch 'origin/main' into new/outline_cfg
acl-cqc Jul 4, 2023
37b2b7a
Spec wording tweaks
acl-cqc Jul 5, 2023
8d0d0b1
And some more.
acl-cqc Jul 5, 2023
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
25 changes: 20 additions & 5 deletions specification/hugr.md
Original file line number Diff line number Diff line change
Expand Up @@ -1226,11 +1226,26 @@ nodes as children.

###### `OutlineCFG`

Replace a CFG-convex subgraph (of sibling BasicBlock nodes) having a
single entry node with a single BasicBlock node having a CFG node child
which has as its children the original BasicBlock nodes and an exit node
that has inputs coming from all edges of the original CFG that don’t
terminate in it.
Replace a set of CFG sibling nodes with a single BasicBlock node having a
CFG node child which has as its children the set of BasicBlock nodes
originally specified. The set of basic blocks must satisfy constraints:
* There must be at most one node in the set with incoming (controlflow) edges
from nodes outside the set. Specifically,
* *either* the set includes the CFG's entry node, and any edges from outside
the set (there may be none or more) target said entry node;
* *or* the set does not include the CFG's entry node, but contains exactly one
node which is the target of edges from outside the set.
* The set may not include the exit block.
* There must be exactly one edge from a node in the set to a node outside it.

Situations in which multiple nodes have edges leaving the set, can be converted to
this form by a combination of InsertIdentity operations and one Replace. Similarly/for example,
acl-cqc marked this conversation as resolved.
Show resolved Hide resolved
rather than movng the Exit block into the nested CFG:
acl-cqc marked this conversation as resolved.
Show resolved Hide resolved
1. an identity node with a single successor can be added onto each edge into the Exit
acl-cqc marked this conversation as resolved.
Show resolved Hide resolved
2. if there is more than one such incoming edge, these identity nodes can then all be combined
acl-cqc marked this conversation as resolved.
Show resolved Hide resolved
by a Replace operation that removes them all and adds a single node (keeping the same number
of in-edges, but reducing to one out-edge to the Exit)
acl-cqc marked this conversation as resolved.
Show resolved Hide resolved
3. The single edge to the Exit node can then be used as the exiting edge.

##### Inlining methods

Expand Down
72 changes: 43 additions & 29 deletions src/algorithm/nest_cfgs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -471,22 +471,15 @@ pub(crate) mod test {
// \-> right -/ \-<--<-/
// Here we would like two consecutive regions, but there is no *edge* between
// the conditional and the loop to indicate the boundary, so we cannot separate them.
let mut cfg_builder = CFGBuilder::new(type_row![NAT], type_row![NAT])?;
let mut entry = cfg_builder.simple_entry_builder(type_row![NAT], 2)?;
let pred_const = entry.add_constant(ConstValue::simple_predicate(0, 2))?; // Nothing here cares which
let const_unit = entry.add_constant(ConstValue::simple_unary_predicate())?;

let entry = n_identity(entry, &pred_const)?;
let merge = build_then_else_merge_from_if(&mut cfg_builder, &const_unit, entry)?;
let tail = build_loop_from_header(&mut cfg_builder, &pred_const, merge)?;
cfg_builder.branch(&merge, 0, &tail)?; // trivial "loop body"
let exit = cfg_builder.exit_block();
cfg_builder.branch(&tail, 0, &exit)?;

let h = cfg_builder.finish_hugr()?;

let (entry, exit) = (entry.node(), exit.node());
let (h, merge, tail) = build_cond_then_loop_cfg(false)?;
let (merge, tail) = (merge.node(), tail.node());
let [entry, exit]: [Node; 2] = h
.children(h.root())
.take(2)
.collect_vec()
.try_into()
.unwrap();

let edge_classes = EdgeClassifier::get_edge_classes(&SimpleCfgView::new(&h));
let [&left,&right] = edge_classes.keys().filter(|(s,_)| *s == entry).map(|(_,t)|t).collect::<Vec<_>>()[..] else {panic!("Entry node should have two successors");};

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

Choose a reason for hiding this comment

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

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

let split = h.output_neighbours(head).exactly_one().unwrap();
// merge is unique predecessor of tail
let merge = h.input_neighbours(tail).exactly_one().unwrap();

let v = SimpleCfgView::new(&h);
let edge_classes = EdgeClassifier::get_edge_classes(&v);
let SimpleCfgView { h: _, entry, exit } = v;
// split is unique successor of head
let split = *edge_classes
.keys()
.filter(|(s, _)| *s == head)
.map(|(_, t)| t)
.exactly_one()
.unwrap();
// merge is unique predecessor of tail
let merge = *edge_classes
.keys()
.filter(|(_, t)| *t == tail)
.map(|(s, _)| s)
.exactly_one()
.unwrap();
let [&left,&right] = edge_classes.keys().filter(|(s,_)| *s == split).map(|(_,t)|t).collect::<Vec<_>>()[..] else {panic!("Split node should have two successors");};
let classes = group_by(edge_classes);
assert_eq!(
Expand Down Expand Up @@ -655,6 +639,36 @@ pub(crate) mod test {
Ok((header, tail))
}

// Result is merge and tail; loop header is (merge, if separate==true; unique successor of merge, if separate==false)
pub fn build_cond_then_loop_cfg(
separate: bool,
) -> Result<(Hugr, BasicBlockID, BasicBlockID), BuildError> {
let mut cfg_builder = CFGBuilder::new(type_row![NAT], type_row![NAT])?;
let mut entry = cfg_builder.simple_entry_builder(type_row![NAT], 2)?;
let pred_const = entry.add_constant(ConstValue::simple_predicate(0, 2))?; // Nothing here cares which
let const_unit = entry.add_constant(ConstValue::simple_unary_predicate())?;

let entry = n_identity(entry, &pred_const)?;
let merge = build_then_else_merge_from_if(&mut cfg_builder, &const_unit, entry)?;
let head = if separate {
let h = n_identity(
cfg_builder.simple_block_builder(type_row![NAT], type_row![NAT], 1)?,
&const_unit,
)?;
cfg_builder.branch(&merge, 0, &h)?;
h
} else {
merge
};
let tail = build_loop_from_header(&mut cfg_builder, &pred_const, head)?;
cfg_builder.branch(&head, 0, &tail)?; // trivial "loop body"
let exit = cfg_builder.exit_block();
cfg_builder.branch(&tail, 0, &exit)?;

let h = cfg_builder.finish_hugr()?;
Ok((h, merge, tail))
}

// Build a CFG, returning the Hugr
pub fn build_conditional_in_loop_cfg(
separate_headers: bool,
Expand Down
50 changes: 50 additions & 0 deletions src/builder/cfg.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use itertools::Itertools;

use super::{
build_traits::SubContainer,
dataflow::{DFGBuilder, DFGWrapper},
Expand Down Expand Up @@ -95,6 +97,22 @@ impl<B: AsMut<Hugr> + AsRef<Hugr>> CFGBuilder<B> {
inputs: Some(input),
})
}

/// Create a CFGBuilder for an existing CFG node (that already has entry + exit nodes)
pub(crate) fn from_existing(base: B, cfg_node: Node) -> Result<Self, BuildError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.)

let OpType::CFG(crate::ops::controlflow::CFG {outputs, ..}) = base.get_optype(cfg_node)
else {return Err(BuildError::UnexpectedType{node: cfg_node, op_desc: "Any CFG"});};
let n_out_wires = outputs.len();
let (_, exit_node) = base.children(cfg_node).take(2).collect_tuple().unwrap();
Ok(Self {
base,
cfg_node,
inputs: None, // This will prevent creating an entry node
exit_node,
n_out_wires,
})
}

/// Return a builder for a non-entry [`BasicBlock::DFB`] child graph with `inputs`
/// and `outputs` and the variants of the branching predicate Sum value
/// specified by `predicate_variants`.
Expand Down Expand Up @@ -278,6 +296,8 @@ impl BlockBuilder<Hugr> {

#[cfg(test)]
mod test {
use std::collections::HashSet;

use cool_asserts::assert_matches;

use crate::builder::build_traits::HugrBuilder;
Expand Down Expand Up @@ -319,6 +339,36 @@ mod test {

Ok(())
}
#[test]
fn from_existing() -> Result<(), BuildError> {
let mut cfg_builder = CFGBuilder::new(type_row![NAT], type_row![NAT])?;
build_basic_cfg(&mut cfg_builder)?;
let h = cfg_builder.finish_hugr()?;

let mut new_builder = CFGBuilder::from_existing(h.clone(), h.root())?;
assert_matches!(new_builder.simple_entry_builder(type_row![NAT], 1), Err(_));
let h2 = new_builder.finish_hugr()?;
assert_eq!(h, h2); // No new nodes added

let mut new_builder = CFGBuilder::from_existing(h.clone(), h.root())?;
let block_builder = new_builder.simple_block_builder(
vec![SimpleType::new_simple_predicate(1), NAT].into(),
type_row![NAT],
1,
)?;
let new_bb = block_builder.container_node();
let [pred, nat]: [Wire; 2] = block_builder.input_wires_arr();
block_builder.finish_with_outputs(pred, [nat])?;
let h2 = new_builder.finish_hugr()?;
let expected_nodes = h
.children(h.root())
.chain([new_bb])
.collect::<HashSet<Node>>();
assert_eq!(expected_nodes, HashSet::from_iter(h2.children(h2.root())));

Ok(())
}

fn build_basic_cfg<T: AsMut<Hugr> + AsRef<Hugr>>(
cfg_builder: &mut CFGBuilder<T>,
) -> Result<(), BuildError> {
Expand Down
1 change: 1 addition & 0 deletions src/hugr/rewrite.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Rewrite operations on the HUGR - replacement, outlining, etc.
pub mod outline_cfg;
pub mod simple_replace;
use std::mem;

Expand Down
Loading