Custom operations implicitly require their extension #388
Assignees
Labels
bug
Something isn't working
Comments
aborgna-q
changed the title
This was referenced Nov 2, 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>
|
Probably blocked by #673 |
acl-cqc
added a commit
that referenced
this issue
Dec 1, 2023
acl-cqc
added a commit
that referenced
this issue
Dec 1, 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
This was referenced Jun 24, 2024
acl-cqc
changed the title
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
From the spec:
The text was updated successfully, but these errors were encountered: