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

[2 of 4] Builder operates on any HugrMut #281

Closed
wants to merge 19 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
4bc534c
[trivial][refactor]Combine identical impl blocks for BlockBuilder
acl-cqc Jul 24, 2023
5dcb6c7
[trivial][refactor]Combine identical impl blocks for TailLoopBuilder
acl-cqc Jul 24, 2023
1dd1f1f
Add Container::Base{Mut,View}, define as &'a (mut) Hugr - build_trait…
acl-cqc Jul 18, 2023
51701cc
WIP (changing region.rs - but needs impl of HugrView that view.rs doe…
acl-cqc Jul 18, 2023
65e02f1
Expose HugrMut; add pub trait Buildable to parametrize all Builder cl…
acl-cqc Jul 24, 2023
e8c0d81
build_traits.rs: allow missing docs
acl-cqc Jul 18, 2023
5e64b0a
clippy (inc 1 where VSCode was wrong)
acl-cqc Jul 19, 2023
8ac5c6b
Revert unnecessary test changes - from_existing(Hugr) still works
acl-cqc Jul 24, 2023
42d5eef
Merge remote-tracking branch 'origin/main' into new/buildable_hugrmut
acl-cqc Jul 25, 2023
46c35a9
deparameterize BaseMut but return &mut; combine impl Buildable for (H…
acl-cqc Jul 25, 2023
0372837
Combine BaseMut/BaseView -> Base; remove explicit drop(!)
acl-cqc Jul 25, 2023
630c216
Buildable does not require HugrMut, but Base constrained Buildable+Hu…
acl-cqc Jul 25, 2023
6ae5516
Container: Buildable. Massive savings on <gunk> albeit deeper refs?
acl-cqc Jul 25, 2023
1742bc9
Buildable::Base does not need to be Buildable!
acl-cqc Jul 25, 2023
2d6ebce
Drop redundant +HugrView constraint, and HugrView impl for (&impl Hug…
acl-cqc Jul 25, 2023
cc8f489
<Self as Buildable>::Base -> B/T (whatever type param is)
acl-cqc Jul 25, 2023
eee773c
Rewrite HugrMut impl using delegate
acl-cqc Jul 25, 2023
f4cc4af
Impl HugrView using delegate
acl-cqc Jul 25, 2023
3cee27a
Reinstate BaseMut<'a>: Buildable+HugrMut and BaseView<'a> -> max 1 in…
acl-cqc Jul 25, 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
13 changes: 8 additions & 5 deletions src/algorithm/nest_cfgs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -397,8 +397,11 @@ impl<T: Copy + Clone + PartialEq + Eq + Hash> EdgeClassifier<T> {
#[cfg(test)]
pub(crate) mod test {
use super::*;
use crate::builder::{BuildError, CFGBuilder, Container, DataflowSubContainer, HugrBuilder};
use crate::builder::{
BuildError, Buildable, CFGBuilder, Container, DataflowSubContainer, HugrBuilder,
};
use crate::hugr::region::{FlatRegionView, Region};
use crate::hugr::HugrMut;
use crate::ops::{
handle::{BasicBlockID, ConstID, NodeHandle},
ConstValue,
Expand Down Expand Up @@ -577,7 +580,7 @@ pub(crate) mod test {
dataflow_builder.finish_with_outputs([u].into_iter().chain(w))
}

fn build_if_then_else_merge<T: AsMut<Hugr> + AsRef<Hugr>>(
fn build_if_then_else_merge<T: Buildable + HugrMut>(
cfg: &mut CFGBuilder<T>,
const_pred: &ConstID,
unit_const: &ConstID,
Expand All @@ -590,7 +593,7 @@ pub(crate) mod test {
Ok((split, merge))
}

fn build_then_else_merge_from_if<T: AsMut<Hugr> + AsRef<Hugr>>(
fn build_then_else_merge_from_if<T: Buildable + HugrMut>(
cfg: &mut CFGBuilder<T>,
unit_const: &ConstID,
split: BasicBlockID,
Expand All @@ -615,7 +618,7 @@ pub(crate) mod test {
}

// Returns loop tail - caller must link header to tail, and provide 0th successor of tail
fn build_loop_from_header<T: AsMut<Hugr> + AsRef<Hugr>>(
fn build_loop_from_header<T: Buildable + HugrMut>(
cfg: &mut CFGBuilder<T>,
const_pred: &ConstID,
header: BasicBlockID,
Expand All @@ -629,7 +632,7 @@ pub(crate) mod test {
}

// Result is header and tail. Caller must provide 0th successor of header (linking to tail), and 0th successor of tail.
fn build_loop<T: AsMut<Hugr> + AsRef<Hugr>>(
fn build_loop<T: Buildable + HugrMut>(
cfg: &mut CFGBuilder<T>,
const_pred: &ConstID,
unit_const: &ConstID,
Expand Down
2 changes: 1 addition & 1 deletion src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pub use handle::BuildHandle;

mod build_traits;
pub use build_traits::{
Container, Dataflow, DataflowHugr, DataflowSubContainer, HugrBuilder, SubContainer,
Buildable, Container, Dataflow, DataflowHugr, DataflowSubContainer, HugrBuilder, SubContainer,
};

mod dataflow;
Expand Down
63 changes: 41 additions & 22 deletions src/builder/build_traits.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#![allow(missing_docs)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤨

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is not for submission, but good to flag up, sorry. If we decide to go this way then I'll take this out and put in docs, but at least it shows that the rest of CI passes

use crate::hugr::validate::InterGraphEdgeError;
use crate::hugr::view::HugrView;
use crate::hugr::{Node, NodeMetadata, Port, ValidationError};
Expand Down Expand Up @@ -29,17 +30,40 @@ use crate::Hugr;

use crate::hugr::HugrMut;

pub trait Buildable {
type BaseMut<'a>: Buildable + HugrMut
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could avoid this Buildable + HugrMut everywhere if we wrote a blanket impl<B:Buildable> HugrMut for B (just by delegating to self.hugr_mut()). That would let us just constrain everything by Buildable instead, which would be nicer. OTOH it means one can execute HugrMut methods on every Buildable, of which there are many (every Container, etc.)....maybe that'd be a good thing (and could be done as a follow-up PR if we want to see what it looks like?)

where
Self: 'a;
type BaseView<'a>: HugrView
where
Self: 'a;
/// The underlying [`Hugr`] being built
fn hugr_mut(&mut self) -> Self::BaseMut<'_>;
/// Immutable reference to HUGR being built
fn hugr(&self) -> Self::BaseView<'_>;
}

impl<T: AsMut<Hugr> + AsRef<Hugr>> Buildable for T {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An interesting alternative (in that it illustrates what's going on) would be one impl Buildable for Hugr and a second impl<T: HugrMut + Copy> Buildable for T - as every &mut is Copy (right?!). That's two impl blocks rather than one though so I didn't...

type BaseMut<'a> = &'a mut Hugr where Self: 'a;

type BaseView<'a> = &'a Hugr where Self: 'a;

fn hugr_mut(&mut self) -> Self::BaseMut<'_> {
self.as_mut()
}

fn hugr(&self) -> Self::BaseView<'_> {
self.as_ref()
}
}

/// Trait for HUGR container builders.
/// Containers are nodes that are parents of sibling graphs.
/// Implementations of this trait allow the child sibling graph to be added to
/// the HUGR.
pub trait Container {
pub trait Container: Buildable {
/// The container node.
fn container_node(&self) -> Node;
/// The underlying [`Hugr`] being built
fn hugr_mut(&mut self) -> &mut Hugr;
/// Immutable reference to HUGR being built
fn hugr(&self) -> &Hugr;
/// Add an [`OpType`] as the final child of the container.
fn add_child_op(&mut self, op: impl Into<OpType>) -> Result<Node, BuildError> {
let parent = self.container_node();
Expand Down Expand Up @@ -79,7 +103,7 @@ pub trait Container {
&mut self,
name: impl Into<String>,
signature: Signature,
) -> Result<FunctionBuilder<&mut Hugr>, BuildError> {
) -> Result<FunctionBuilder<Self::BaseMut<'_>>, BuildError> {
let f_node = self.add_child_op(ops::FuncDefn {
name: name.into(),
signature: signature.clone(),
Expand Down Expand Up @@ -252,7 +276,7 @@ pub trait Dataflow: Container {
&mut self,
signature: Signature,
input_wires: impl IntoIterator<Item = Wire>,
) -> Result<DFGBuilder<&mut Hugr>, BuildError> {
) -> Result<DFGBuilder<Self::BaseMut<'_>>, BuildError> {
let (dfg_n, _) = add_op_with_wires(
self,
ops::DFG {
Expand All @@ -278,7 +302,7 @@ pub trait Dataflow: Container {
&mut self,
inputs: impl IntoIterator<Item = (SimpleType, Wire)>,
output_types: SimpleRow,
) -> Result<CFGBuilder<&mut Hugr>, BuildError> {
) -> Result<CFGBuilder<Self::BaseMut<'_>>, BuildError> {
let (input_types, input_wires): (Vec<SimpleType>, Vec<Wire>) = inputs.into_iter().unzip();

let inputs: SimpleRow = input_types.into();
Expand Down Expand Up @@ -337,7 +361,7 @@ pub trait Dataflow: Container {
just_inputs: impl IntoIterator<Item = (ClassicType, Wire)>,
inputs_outputs: impl IntoIterator<Item = (SimpleType, Wire)>,
just_out_types: ClassicRow,
) -> Result<TailLoopBuilder<&mut Hugr>, BuildError> {
) -> Result<TailLoopBuilder<Self::BaseMut<'_>>, BuildError> {
let (input_types, mut input_wires): (Vec<ClassicType>, Vec<Wire>) =
just_inputs.into_iter().unzip();
let (rest_types, rest_input_wires): (Vec<SimpleType>, Vec<Wire>) =
Expand Down Expand Up @@ -371,7 +395,7 @@ pub trait Dataflow: Container {
(predicate_inputs, predicate_wire): (impl IntoIterator<Item = ClassicRow>, Wire),
other_inputs: impl IntoIterator<Item = (SimpleType, Wire)>,
output_types: SimpleRow,
) -> Result<ConditionalBuilder<&mut Hugr>, BuildError> {
) -> Result<ConditionalBuilder<Self::BaseMut<'_>>, BuildError> {
let mut input_wires = vec![predicate_wire];
let (input_types, rest_input_wires): (Vec<SimpleType>, Vec<Wire>) =
other_inputs.into_iter().unzip();
Expand Down Expand Up @@ -526,9 +550,7 @@ pub trait Dataflow: Container {
function: &FuncID<DEFINED>,
input_wires: impl IntoIterator<Item = Wire>,
) -> Result<BuildHandle<DataflowOpID>, BuildError> {
let hugr = self.hugr();
let def_op = hugr.get_optype(function.node());
let signature = match def_op {
let signature = match self.hugr().get_optype(function.node()) {
OpType::FuncDefn(ops::FuncDefn { signature, .. })
| OpType::FuncDecl(ops::FuncDecl { signature, .. }) => signature.clone(),
_ => {
Expand All @@ -540,7 +562,7 @@ pub trait Dataflow: Container {
};
let const_in_port = signature.output.len();
let op_id = self.add_dataflow_op(ops::Call { signature }, input_wires)?;
let src_port = self.hugr_mut().num_outputs(function.node()) - 1;
let src_port = self.hugr().num_outputs(function.node()) - 1;

self.hugr_mut()
.connect(function.node(), src_port, op_id.node(), const_in_port)?;
Expand Down Expand Up @@ -586,9 +608,10 @@ fn wire_up_inputs<T: Dataflow + ?Sized>(
dst_port,
)?;
}
let base = data_builder.hugr_mut();
let base = data_builder.hugr();
let op = base.get_optype(op_node);
let some_df_outputs = !op.signature().output.is_empty();
drop(base);
if !any_local_df_inputs && some_df_outputs {
// If op has no inputs add a StateOrder edge from input to place in
// causal cone of Input node
Expand All @@ -605,7 +628,7 @@ fn wire_up<T: Dataflow + ?Sized>(
dst: Node,
dst_port: usize,
) -> Result<bool, BuildError> {
let base = data_builder.hugr_mut();
let mut base = data_builder.hugr_mut();
let src_offset = Port::new_outgoing(src_port);

let src_parent = base.get_parent(src);
Expand Down Expand Up @@ -652,14 +675,10 @@ fn wire_up<T: Dataflow + ?Sized>(
}
}

data_builder
.hugr_mut()
.connect(src, src_port, dst, dst_port)?;
base.connect(src, src_port, dst, dst_port)?;
Ok(local_source
&& matches!(
data_builder
.hugr_mut()
.get_optype(dst)
base.get_optype(dst)
.port_kind(Port::new_incoming(dst_port))
.unwrap(),
EdgeKind::Value(_)
Expand Down
49 changes: 28 additions & 21 deletions src/builder/cfg.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use itertools::Itertools;

use super::{
build_traits::SubContainer,
build_traits::{Buildable, SubContainer},
dataflow::{DFGBuilder, DFGWrapper},
handle::BuildHandle,
BasicBlockID, BuildError, CfgID, Container, Dataflow, HugrBuilder, Wire,
Expand All @@ -25,24 +25,28 @@ pub struct CFGBuilder<T> {
pub(super) n_out_wires: usize,
}

impl<B: AsMut<Hugr> + AsRef<Hugr>> Container for CFGBuilder<B> {
impl<B: Buildable> Buildable for CFGBuilder<B> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are, indeed, a lot of impl blocks like this. I guess these could be done via a macro...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, if/presuming that works, one could have a macro implementing HugrMut for all of these, which would allow combining Buildable into HugrMut. (If we want HugrMut-ness spread so widely).

type BaseMut<'a> = B::BaseMut<'a> where Self: 'a;
type BaseView<'a> = B::BaseView<'a> where Self: 'a;
#[inline]
fn container_node(&self) -> Node {
self.cfg_node
fn hugr_mut(&mut self) -> Self::BaseMut<'_> {
self.base.hugr_mut()
}

#[inline]
fn hugr_mut(&mut self) -> &mut Hugr {
self.base.as_mut()
fn hugr(&self) -> Self::BaseView<'_> {
self.base.hugr()
}
}

impl<B: Buildable + HugrMut> Container for CFGBuilder<B> {
#[inline]
fn hugr(&self) -> &Hugr {
self.base.as_ref()
fn container_node(&self) -> Node {
self.cfg_node
}
}

impl<H: AsMut<Hugr> + AsRef<Hugr>> SubContainer for CFGBuilder<H> {
impl<B: Buildable + HugrMut> SubContainer for CFGBuilder<B> {
type ContainerHandle = BuildHandle<CfgID>;
#[inline]
fn finish_sub_container(self) -> Result<Self::ContainerHandle, BuildError> {
Expand Down Expand Up @@ -76,7 +80,7 @@ impl HugrBuilder for CFGBuilder<Hugr> {
}
}

impl<B: AsMut<Hugr> + AsRef<Hugr>> CFGBuilder<B> {
impl<B: Buildable + HugrMut> CFGBuilder<B> {
pub(super) fn create(
mut base: B,
cfg_node: Node,
Expand All @@ -88,7 +92,7 @@ impl<B: AsMut<Hugr> + AsRef<Hugr>> CFGBuilder<B> {
cfg_outputs: output,
});
let exit_node = base
.as_mut()
.hugr_mut()
.add_op_with_parent(cfg_node, exit_block_type)?;
Ok(Self {
base,
Expand Down Expand Up @@ -126,7 +130,7 @@ impl<B: AsMut<Hugr> + AsRef<Hugr>> CFGBuilder<B> {
inputs: SimpleRow,
predicate_variants: Vec<ClassicRow>,
other_outputs: SimpleRow,
) -> Result<BlockBuilder<&mut Hugr>, BuildError> {
) -> Result<BlockBuilder<<Self as Buildable>::BaseMut<'_>>, BuildError> {
self.any_block_builder(inputs, predicate_variants, other_outputs, false)
}

Expand All @@ -136,7 +140,7 @@ impl<B: AsMut<Hugr> + AsRef<Hugr>> CFGBuilder<B> {
predicate_variants: Vec<ClassicRow>,
other_outputs: SimpleRow,
entry: bool,
) -> Result<BlockBuilder<&mut Hugr>, BuildError> {
) -> Result<BlockBuilder<<Self as Buildable>::BaseMut<'_>>, BuildError> {
let op = OpType::BasicBlock(BasicBlock::DFB {
inputs: inputs.clone(),
other_outputs: other_outputs.clone(),
Expand Down Expand Up @@ -170,7 +174,7 @@ impl<B: AsMut<Hugr> + AsRef<Hugr>> CFGBuilder<B> {
inputs: SimpleRow,
outputs: SimpleRow,
n_cases: usize,
) -> Result<BlockBuilder<&mut Hugr>, BuildError> {
) -> Result<BlockBuilder<<Self as Buildable>::BaseMut<'_>>, BuildError> {
self.block_builder(inputs, vec![type_row![]; n_cases], outputs)
}

Expand All @@ -185,7 +189,7 @@ impl<B: AsMut<Hugr> + AsRef<Hugr>> CFGBuilder<B> {
&mut self,
predicate_variants: Vec<ClassicRow>,
other_outputs: SimpleRow,
) -> Result<BlockBuilder<&mut Hugr>, BuildError> {
) -> Result<BlockBuilder<<Self as Buildable>::BaseMut<'_>>, BuildError> {
let inputs = self
.inputs
.take()
Expand All @@ -203,7 +207,7 @@ impl<B: AsMut<Hugr> + AsRef<Hugr>> CFGBuilder<B> {
&mut self,
outputs: SimpleRow,
n_cases: usize,
) -> Result<BlockBuilder<&mut Hugr>, BuildError> {
) -> Result<BlockBuilder<<Self as Buildable>::BaseMut<'_>>, BuildError> {
self.entry_builder(vec![type_row![]; n_cases], outputs)
}

Expand Down Expand Up @@ -232,7 +236,7 @@ impl<B: AsMut<Hugr> + AsRef<Hugr>> CFGBuilder<B> {
/// Builder for a [`BasicBlock::DFB`] child graph.
pub type BlockBuilder<B> = DFGWrapper<B, BasicBlockID>;

impl<B: AsMut<Hugr> + AsRef<Hugr>> BlockBuilder<B> {
impl<B: Buildable + HugrMut> BlockBuilder<B> {
/// Set the outputs of the block, with `branch_wire` being the value of the
/// predicate. `outputs` are the remaining outputs.
pub fn set_outputs(
Expand All @@ -242,6 +246,7 @@ impl<B: AsMut<Hugr> + AsRef<Hugr>> BlockBuilder<B> {
) -> Result<(), BuildError> {
Dataflow::set_outputs(self, [branch_wire].into_iter().chain(outputs.into_iter()))
}

fn create(
base: B,
block_n: Node,
Expand All @@ -257,8 +262,7 @@ impl<B: AsMut<Hugr> + AsRef<Hugr>> BlockBuilder<B> {
let db = DFGBuilder::create_with_io(base, block_n, signature)?;
Ok(BlockBuilder::from_dfg_builder(db))
}
}
impl<B: AsMut<Hugr> + AsRef<Hugr>> BlockBuilder<B> {

/// [Set outputs](BlockBuilder::set_outputs) and [finish](`BlockBuilder::finish_sub_container`).
pub fn finish_with_outputs(
mut self,
Expand Down Expand Up @@ -371,9 +375,12 @@ mod test {
Ok(())
}

fn build_basic_cfg<T: AsMut<Hugr> + AsRef<Hugr>>(
fn build_basic_cfg<T: Buildable + HugrMut>(
cfg_builder: &mut CFGBuilder<T>,
) -> Result<(), BuildError> {
) -> Result<(), BuildError>
where
CFGBuilder<T>: Container,
{
let sum2_variants = vec![
classic_row![ClassicType::i64()],
classic_row![ClassicType::i64()],
Expand Down
Loading