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

Remove single source requirement for DSGs #743

Closed
ss2165 opened this issue Dec 15, 2023 · 7 comments
Closed

Remove single source requirement for DSGs #743

ss2165 opened this issue Dec 15, 2023 · 7 comments
Labels
spec Issues to do with the specification document(s)
Milestone

Comments

@ss2165
Copy link
Member

ss2165 commented Dec 15, 2023

from @cqc-alec :

The spec currently states that "In a data dependency subgraph, a valid ordering of operations can be achieved by topologically sorting the nodes starting from Input with respect to the Value and Order edges." I vote we drop this requirement, since it places unnecessary constraints on execution order (there's no reason why a LoadConstant operation shouldn't be executed before knowing the inputs to a DFG). If efficiency of topsort is a big concern we can find other ways to optimize it.

@ss2165 ss2165 added P-high spec Issues to do with the specification document(s) labels Dec 15, 2023
@aborgna-q aborgna-q added this to the Release 0.1.0 milestone Jan 3, 2024
@cqc-alec
Copy link
Collaborator

cqc-alec commented Jan 4, 2024

I believe this was fixed in the spec by #762 , but the implementation still needs updating (to not add the unnecessary Order edges).

@acl-cqc
Copy link
Contributor

acl-cqc commented Jan 4, 2024

Do we actually have any implementation code adding such Order edges? I check e.g.

fn add_constant(&mut self, constant: impl Into<ops::Const>) -> Result<ConstID, BuildError> {
let const_n = self.add_child_node(NodeType::new(constant.into(), ExtensionSet::new()))?;
Ok(const_n.into())
}
and
fn load_const(&mut self, cid: &ConstID) -> Result<Wire, BuildError> {
let const_node = cid.node();
let nodetype = self.hugr().get_nodetype(const_node);
let op: ops::Const = nodetype
.op()
.clone()
.try_into()
.expect("ConstID does not refer to Const op.");
let load_n = self.add_dataflow_op(
ops::LoadConstant {
datatype: op.const_type().clone(),
},
// Constant wire from the constant value node
vec![Wire::new(const_node, OutgoingPort::from(0))],
)?;
Ok(load_n.out_wire(0))
}
in the builder, and don't see anything; and we haven't implemented InsertConst(Ignore) rewrites...

@cqc-alec
Copy link
Collaborator

cqc-alec commented Jan 4, 2024

I may be wrong about that, but my assumption was based on comments @peter-campora made a few weeks ago where he was surprised by the presence of some Order edges.

@cqc-alec
Copy link
Collaborator

cqc-alec commented Jan 4, 2024

I can't find anywhere in the code where we do this either.

@ss2165
Copy link
Member Author

ss2165 commented Jan 4, 2024

Builder used to do this but was removed in this commit:

c7bfffe#diff-7b05863e5e5858f70dcaba4fa126e7782aa9b8da6fd64b80715d821b9867bef0L634-L641

@cqc-alec
Copy link
Collaborator

cqc-alec commented Jan 4, 2024

OK! So can we close this issue?

@ss2165
Copy link
Member Author

ss2165 commented Jan 4, 2024

Looks like it, the validation code does not depend on it either

hugr/src/hugr/validate.rs

Lines 376 to 377 in 0c910cf

let postorder = Topo::new(&region.as_petgraph());
let nodes_visited = postorder

@ss2165 ss2165 closed this as completed Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec Issues to do with the specification document(s)
Projects
None yet
Development

No branches or pull requests

4 participants