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

Infer extension-requirements for nested DFGs #640

Closed
Tracked by #426
acl-cqc opened this issue Nov 4, 2023 · 3 comments
Closed
Tracked by #426

Infer extension-requirements for nested DFGs #640

acl-cqc opened this issue Nov 4, 2023 · 3 comments
Assignees

Comments

@acl-cqc
Copy link
Contributor

acl-cqc commented Nov 4, 2023

Nodes such as DFG, CFG, BasicBlock etc. are created wth a FunctionType that specifies a delta:

pub extension_reqs: ExtensionSet,

This must be specified explicitly at creation time - one cannot say "None" as one can for input_extensions. However it would be nicer if one could, such that the inference algorithm then infers it, which I believe is possible as follows.

(This would require some change to the FunctionType/NodeType/DFG structures to allow specifying an Option, and then mutating it when inference is complete, in discussion below)

During inference, we both 1. constraint the input+output variables to match those at the Input and Output nodes of the DFG:

hugr/src/extension/infer.rs

Lines 283 to 288 in 1a07cd9

if let Some([input, output]) = hugr.get_io(node) {
for dir in Direction::BOTH {
let m_input_node = self.make_or_get_meta(input, dir);
self.add_constraint(m_input_node, Constraint::Equal(m_input));
let m_output_node = self.make_or_get_meta(output, dir);
self.add_constraint(m_output_node, Constraint::Equal(m_output));
and 2. constrain the outputs to be the input plus the specified delta:

hugr/src/extension/infer.rs

Lines 314 to 318 in 1a07cd9

self.gen_union_constraint(
m_input,
m_output,
node_type.op_signature().extension_reqs,
);

If we simplify avoid the second constraints in such cases, the constraints with the Input/Output nodes should be sufficient to calculate both. We can then compute the delta as the difference between the two. (If this last seems dubious - indeed the delta could also include any subset of the inputs - then this is no worse than at present, where such a delta specified by the programmer would be accepted by inference and validation. We could instead move to a system where the Input node also has empty input extensions, but that's another issue.)

@acl-cqc
Copy link
Contributor Author

acl-cqc commented Jan 5, 2024

Thus, two possible solutions:

  • Recursively infer delta from the nested subgraph first, use that in the outer graph. This won't handle functions containing recursive calls (you need the delta from the function body to infer around the call node) - user-provided annotations on functions?
  • Try to generalize "Plus" constraint to merge two variables. This might magically work or break everything, not sure. Maybe it's no worse than what we have now, but likely impossible to solve such a constraint backwards (as per Inference Redux #780 )

@acl-cqc
Copy link
Contributor Author

acl-cqc commented May 30, 2024

This would require some change to the FunctionType/NodeType/DFG structures to allow specifying an Option, and then mutating it when inference is complete

So it's worth thinking about this because it could be quite disruptive. There are several possible methods, including:

  1. Change FunctionType to store an Option<ExtensionSet>. Then validation checks all these (including those nested within port/edge types) are non-None. However it's not clear that None is useful in many places, e.g. outside of a node's signature(). Moreover having to deal with Option rather than ExtensionSet everywhere one has a FunctionType makes this seem like the most disruptive method.
  2. Separate Node signature() from FunctionType; the former has an Option, the latter a non-optional ExtensionSet. Given we are working on Separate type of node signature from function value #1094 (e.g. refactor!: Separate Signature from FuncValueType by parametrizing Type(/Row)/etc. #1138) this seems possible but could be very complex.
  3. Leave all signatures/FunctionTypes containing an ExtensionSet, but allow inference to mutate it (!). There are are few further variants...
    • Allow any container node's extension_reqs to be mutated. This means that extension-delta-inference will never fail, but seems a bit wild.
    • Allow, say, only container extension_reqs that are currently empty to be inferred.
    • Restrict inference to e.g. only increasing extension_reqs, or only reducing them.
    • As a variant on the latter, suppose some kind of special ExtensionId wildcard/universal-set, which could be seen as meaning either "all possible extensions" or "please infer me". An ExtensionSet containing both the special please-infer and other ExtensionIds coould mean "at least the others, possibly more". We might want to make the default ExtensionSet contain only the wildcard, and (even more likely) make the builder default to using just the wildcard.
  4. Remove extension_reqs from FunctionType, so the latter is just inputs/outputs; then a Type(Enum)::Function stores both a FunctionType (in/out) and a non-optional ExtensionSet; whereas a node has a "signature() -> FunctionType" and a separate extension_delta: Option<ExtensionSet> that can be set (from None) by inference. [Yes, this is a different spin on 2., but it might be a good way of decoupling the delta from the FunctionType-does/doesn't-have-rowvariables question.]

github-merge-queue bot pushed a commit that referenced this issue Jun 11, 2024
…RIP inference (#1142)

This is probably phase 1 of 4, later steps being
* Remove `input_extensions` and `NodeType`
* Make every operation require it's own extension (#388)
* Infer deltas for parent nodes (#640) - this may be disruptive, and/or
take some subtlety, as currently every FunctionType stores an
ExtensionSet not an Option thereof
* Finally, remove the feature flag

So, this PR updates validation to ignore the input_extensions, and
removes the inference algorithm that would set them. There were a few
complications:
* A lot of tests create a DFGBuilder with an *empty* delta, then put
things in it that have extension requirements. The inference algorithm
figures out that this is all OK if it can just put that
extension-requirement on the `input_extensions` to the DFG node (root)
itself. So this might be a call for `input_extensions` - they do reduce
the need for #640 a bit.
* We can see roughly how painful life is (without delta-inference nor
input-extensions) looking at the various extra extension-deltas I've had
to specify here
* I think that given we are under the feature flag at the moment, we are
OK to continue, however the need for #640 is now somewhat increased, so
discussion there strongly encouraged, please! :)
* `TailLoop` turned out not to have an extension-delta, where it clearly
needs one (just as DataflowBlock and others). Also there was an
implementation of `OpParent` for it, whereas in fact we needed
`DataflowParent` as that would also have got us the `ValidateOp` for
free. This all in 08bb5a5.
* Another bug in DataflowBlock::inner_signature, and some others.

....That is to say, in some ways this validation scheme is *stricter*
than what we had; there is both the slight flexibility that
`input_extensions` gave us in mis-specifying deltas, and that this
scheme's simplicity makes it much harder to accidentally omit cases from
validation....

I've also kept `Hugr::infer_extensions` around as a no-op rather than
remove it and later bring it back (in #640). Maybe we should remove it,
but that would be a breaking change....OTOH I've removed
`validate_with_extension_closure` and in theory we could have that (a
closure containing deltas for nested DFGs) too...

BREAKING CHANGE: TailLoop node and associated builder functions now
require specifying an ExtensionSet; extension/validate.rs deleted; some
changes to Hugrs validated/rejected when the `extension_inference`
feature flag is turned on
@acl-cqc acl-cqc self-assigned this Jun 13, 2024
github-merge-queue bot pushed a commit that referenced this issue Jun 24, 2024
…ck, Dfg, TailLoop (#1195)

closes #640 
* Add an ExtensionId TO_BE_INFERRED
* On Case, Cfg, Conditional, DataflowBlock, Dfg, TailLoop,
`Hugr::infer_extensions` replaces TO_BE_INFERRED with the smallest set
of ExtensionIds that's correct wrt. its child nodes (i.e., the union of
the child node deltas). If there are other ExtensionIds alongside
TO_BE_INFERRED these will be kept (allowing a "lower bound" to be
specified for individual nodes)
* `Hugr::infer_extensions` also takes a `remove: bool` which, if true,
modifies deltas of the same OpTypes that do *not* have TO_BE_INFERRED,
by removing ExtensionIds. (Thus, non-inferred deltas act as upper
bounds).

BREAKING CHANGE: ExtensionError moved from hugr::validate to hugr;
Hugr::infer_extensions takes bool parameter
github-merge-queue bot pushed a commit that referenced this issue Jun 24, 2024
…ck, Dfg, TailLoop (#1195)

* Add an ExtensionId TO_BE_INFERRED
* On Case, Cfg, Conditional, DataflowBlock, Dfg, TailLoop,
`Hugr::infer_extensions` replaces TO_BE_INFERRED with the smallest set
of ExtensionIds that's correct wrt. its child nodes (i.e., the union of
the child node deltas). If there are other ExtensionIds alongside
TO_BE_INFERRED these will be kept (allowing a "lower bound" to be
specified for individual nodes)
* `Hugr::infer_extensions` also takes a `remove: bool` which, if true,
modifies deltas of the same OpTypes that do *not* have TO_BE_INFERRED,
by removing ExtensionIds. (Thus, non-inferred deltas act as upper
bounds).

Relates to #640 but does not address the builder where this should be
the default behaviour.

BREAKING CHANGE: ExtensionError moved from hugr::validate to hugr;
Hugr::infer_extensions takes bool parameter
@acl-cqc
Copy link
Contributor Author

acl-cqc commented Jun 28, 2024

Closing after #1195, front-end/builder changes can be separate issue (and #1237 is a separate issue!)

@acl-cqc acl-cqc closed this as completed Jun 28, 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