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

Custom operations implicitly require their extension #388

Closed
Tracked by #426
aborgna-q opened this issue Aug 9, 2023 · 1 comment · Fixed by #1226
Closed
Tracked by #426

Custom operations implicitly require their extension #388

aborgna-q opened this issue Aug 9, 2023 · 1 comment · Fixed by #1226
Assignees
Labels
bug Something isn't working

Comments

@aborgna-q
Copy link
Collaborator

aborgna-q commented Aug 9, 2023

From the spec:

All operations declared as part of a resource interface implicitly have the resource they pertain to as a requirement.

@aborgna-q aborgna-q added the bug Something isn't working label Aug 9, 2023
@aborgna-q aborgna-q self-assigned this Aug 9, 2023
@aborgna-q aborgna-q changed the title Custom operations just implicitly require their resource Custom operations implicitly require their resource Aug 29, 2023
@croyzor croyzor self-assigned this Sep 20, 2023
github-merge-queue bot pushed a commit that referenced this issue Nov 7, 2023
closes #633

* Add test case. Reduced this to a 2-node DFG (plus Input and Output).
That this hasn't been showing up repeatedly in other tests - I put down
to #388: most custom ops have empty `extension_reqs` so do not generate
Plus constraints.
* Run (50%-failing) test 10 times -> failure rate ~~ 1 in 2^10
* Fix calculation of `live_vars` and `live_metas` by one-off traversal
of constraints+solutions in `fn results()`.

---------

Co-authored-by: Craig Roy <craig.roy@cambridgequantum.com>
@acl-cqc
Copy link
Contributor

acl-cqc commented Nov 10, 2023

Probably blocked by #673

@acl-cqc acl-cqc self-assigned this Dec 2, 2023
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.
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 changed the title Custom operations implicitly require their resource Custom operations implicitly require their extension Jul 3, 2024
github-merge-queue bot pushed a commit that referenced this issue Jul 15, 2024
Includes adding new variants of `block_builder`, `entry_builder`,
`simple_entry_builder` and `conditional_builder`: the default version
omits the extension set parameter, the `_exts` variant takes an extra
parameter (being an ExtensionSet). `simple_block_builder` is untouched
(as it takes a FunctionType, so can use `endo_ft`/`inout_ft`)

We'll need similar updates to `cfg_builder`, `tail_loop_builder`,
`ConditionalBuilder::new` and `TailLoopBuilder::new` but I'll leave
those for another PR, there's quite enough here ;)

closes #388

BREAKING CHANGE: (1) container-node extension-deltas will need to be
enlarged to include ops therein; for FuncDefn this will have to be
manually specified but for other containers TO_BE_INFERRED, `endo_ft` or
`inout_ft` all work. (2) `block_builder`, `entry_builder`,
`simple_entry_builder` and `conditional_builder` no longer take an
ExtensionSet; either drop the argument or use the `..._exts` variant.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment