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

Extension Inference for functions relies on builder #702

Closed
acl-cqc opened this issue Nov 16, 2023 · 2 comments
Closed

Extension Inference for functions relies on builder #702

acl-cqc opened this issue Nov 16, 2023 · 2 comments
Assignees

Comments

@acl-cqc
Copy link
Contributor

acl-cqc commented Nov 16, 2023

When defining a function, the builder fixes solutions for the extensions of the Input and Output child of the FuncDefn:

let db = DFGBuilder::create_with_io(
self.hugr_mut(),
f_node,
signature.signature,
Some(signature.input_extensions),
(note last line)
.add_node_with_parent(parent, NodeType::new(input, input_extensions.clone()))?;
base.as_mut().add_node_with_parent(
parent,
NodeType::new(
output,
input_extensions.map(|inp| inp.union(&signature.extension_reqs)),
(note the two input_extensions)

The relationship between Input and Output is not modelled by inference - they should have a Plus constraint, but Plus constraints are only generated here:

hugr/src/extension/infer.rs

Lines 317 to 330 in 41e15da

match node_type.io_extensions() {
// Input extensions are open
None => {
let c = if let Some(sig) = node_type.op_signature() {
let delta = sig.extension_reqs;
if delta.is_empty() {
Constraint::Equal(m_input)
} else {
Constraint::Plus(delta, m_input)
}
} else {
Constraint::Equal(m_input)
};
self.add_constraint(m_output, c);
(note this only applies if the node has an op_signature, which FuncDefn doesn't, and only if extension-annotations need to be inferred; if they are pre-specified, inference just accepts them)....and neither is it checked by validation, as the only checks on Input/Output are done here:

hugr/src/hugr/validate.rs

Lines 205 to 212 in 41e15da

if node_type.tag() != OpTag::FuncDefn {
// If this is a container with I/O nodes, check that the extension they
// define match the extensions of the container.
if let Some([input, output]) = self.hugr.get_io(node) {
self.extension_validator
.validate_io_extensions(node, input, output)?;
}
}

(note FuncDefns are explicitly excluded, because validate_io_children would use the wrong delta for them).

Thus, it should be possible to construct a Hugr (not using the builder) where the body of the function adds extension-requirements not specified in the function delta, and to get this past validation (possibly past inference too, but otherwise by hand-specifying extensions). Step 1, obviously, produce that test case ;-)

@acl-cqc
Copy link
Contributor Author

acl-cqc commented Nov 16, 2023

May be best to do this after #692 where the builder changes to pass None (no extensions specified) to create_with_io

github-merge-queue bot pushed a commit that referenced this issue Dec 12, 2023
* Make each Value able to report its ExtensionSet needed at runtime (and
hence, also each CustomConst)
* Also give OpTrait a method for the extension-delta. This is the delta
of the FunctionType, *for dataflow ops*, but can be defined for other
optypes too - specifically (here), Const ops and also Case. Use this in
both inference and for `NodeType::io_extensions()`
* Input extensions of every Const node can now be the empty set (i.e.
the default-for-ModuleOp `pure`), as the relevant extension will be
added in the delta - thus, drop separate extension parameter in builder
(add_constant, add_load_const, etc.). LoadConstant ops can now also have
an empty delta and open input-extensions as these can be figured out
from the Const. This is thus a step towards fixing #702...
* Also note slight issue in replace::test::cfg, pending #388
* Add `ExtensionSet::union_over(impl IntoIterator<Item=Self>) -> Self`
utility method

This should ease the way towards solving the rest of #702 and moreover
to removing `new_auto` - we should be able to constrain the
input-extensions for any ModuleOp to `pure` in inference and thus
*every* node can be created open rather than a mix, but those are all
for follow-up PRs.
@acl-cqc acl-cqc self-assigned this Jan 24, 2024
@acl-cqc
Copy link
Contributor Author

acl-cqc commented Jun 13, 2024

I think we can say this was done in #1142 / #1183

@acl-cqc acl-cqc closed this as completed Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant