CQCL / hugr

Hierarchical Unified Graph Representation for quantum and classical programs
https://crates.io/crates/hugr
Apache License 2.0
16 stars 4 forks source link

`CustomTypeArg` should hold a `Value` #1308

Closed doug-q closed 1 month ago

doug-q commented 1 month ago

Currently CustomTypeArg holds a serde_yaml::Value payload. We want to change this to a serde_json::Value https://github.com/CQCL/hugr/issues/1048 , I suggest instead we should change it to crate::ops::custom::Value

acl-cqc commented 1 month ago

Discussed this quite a bit with @ss2165. Thoughts

This leads to a design space including options like....

  1. Make CustomTypeArg contain a Value, but rule out CustomTypeArg as a TypeArg to Type(Def), and allow it only as an arg to an OpDef. (TypeArgs given to an OpDef can include Types, but TypeArg's inside those should not include CustomTypeArg either. So it's a bit like row variables, the top level is different, but reversed - CustomTypeArg is extra at the top level for OpDefs, whereas RowVariables are ruled out at the top level for nodes.)
    • This means the CustomTypeArg's are passed to compute_signature, which might be essential, but sounds a bit wild if not essential (and we think the latter?)
  2. Remove CustomTypeArg altogether; add a separate mechanism for an OpDef to have "static parameters" (NOT static type parameters) that are values. These have declared custom types (e.g. float for a static non-computed angle).
    • Preferably, they are not passed to compute_signature, but possibly compute_signature can return a specification of what static params the op should take.
    • Note however that if we want to be able to substitute in angles specified as post-compilation runtime parameters ("command-line arguments"), that means angle must be a node/ExternalOp fed in as a runtime param, not a "static parameter", so angle is not a good example here.
    • This is nicer: Type and TypeArg can be Eq (without cheating), Op and static-params are not. But, more change....
doug-q commented 1 month ago

I mostly agree with Alan above, CustomTypeArgs are too powerful and should be weakened. The impl Eq is dodgy, and we should fix this. Indeed I want to add Hash #1329 .

I was of the view that it is important to retain the ability for compute_signature to depend on some arbitrary user defined data, but I have changed my mind. If you wanted to do that, you could always add a Param of FunctionType, or two List params that specified the signature.

I do think static Value parameters would be somewhat nicer.

Use case for compute_signature depending on arbitrary user defined data:

We will need to support qir -> hugr to run tket optimisations on qir. The simplest way I can see to do this is to define a llvm.swiss_army_knife op that takes some ad-hoc JSON

ss2165 commented 1 month ago

I do think static Value parameters would be somewhat nicer.

I agree. Should we be attaching this to nodes or rely on our existing infrastructure for this - i.e. all ops can have static edges?

Another idea: add a value->static op that has to be removed at runtime, potentially gives us way more powerful constexpr handling?

(I think Zig does this by emulating the target but how we actually implement the evaluation is not important here I think)

acl-cqc commented 1 month ago

We will need to support qir -> hugr to run tket optimisations on qir. The simplest way I can see to do this is to define a llvm.swiss_army_knife op that takes some ad-hoc JSON

...and computes a type from it?

Yeah, ugh, I mean, we ought to have some backdoor way to do that - it does not necessarily have to be nice or easy...

For example, having to pass the desired FunctionType as well as the "ad-hoc JSON", would be fine, but I'd still want to be able to call extension code to validate that the FunctionType is correct wrt. the JSON, and since the compute_signature interface doesn't distinguish between validating and re-computing, you'd still just need to be calling that with both FunctionType and JSON....in which case you don't need to pass the former...

I love @ss2165's "take extra Static args" approach but that doesn't get any extra data into compute_signature either. If we really need that then either...we add extra args to compute_signature (somehow, with potentially-annoying breakage); add an extra validate_signature function that takes the type args as well as the static params (a bit ugh, I think, but more backwards-compatible); or pass the desired type explicitly in the TypeArgs and delay the point of validation until inside some extension-understanding tool. None of these seems great although I wouldn't say any were terrible either. Thoughts?

ss2165 commented 1 month ago

Lots of nice follow ups from the discussion above, but for now I'm going with replacing OpaqueArg with a string, and that should be addressing the original issue.

ss2165 commented 1 month ago

Discussion on static-edge parameters moved to #1342