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

Allow custom ops with static constant input edges #1594

Closed
ss2165 opened this issue Oct 18, 2024 Discussed in #1342 · 6 comments
Closed

Allow custom ops with static constant input edges #1594

ss2165 opened this issue Oct 18, 2024 Discussed in #1342 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@ss2165
Copy link
Member

ss2165 commented Oct 18, 2024

Discussed in #1342

Originally posted by ss2165 July 24, 2024
This follows discussions mainly in #1308

A constant static input to an op would would be some semantically significant data, that cannot be lost like metadata. This could be thought of as "literal properties" or "intrinsic parameters".

If further we can elevate values to constants at compile time (constexpr evaluation) we can do powerful things with ops that need some structured, complicated, compile-time known data.

An open question is whether this data is available to binary compute_signature functions.

@doug-q
Copy link
Collaborator

doug-q commented Oct 23, 2024

An open question is whether this data is available to binary compute_signature functions.

Currently compute_signature takes [TypeParam] -> PolyFuncType. I suggest that with this change it should take [TypeParam] -> ([Type], PolyFuncType), where the list [Type] specifies the number, order, and types of required static input edges. Ideally any in vars the list should resolve in the scope of the type params inside the PolyFuncType.

With the feature as described it is not allowed for extension operations to take Function edges from FuncDefn/FuncDecl, because those edges are polymorphic, unlike the Static edges from Const nodes. The following change would lift this restriction:

Rename LoadFunction to TypeApply. Currently LoadFunction has a single output Value edge. Change this to a Const edge.

@acl-cqc
Copy link
Contributor

acl-cqc commented Oct 23, 2024

Rename LoadFunction to TypeApply. Currently LoadFunction has a single output Value edge. Change this to a Const edge.

So LoadFunction is now static-function to static-const. This can wire into a LoadConstant if you want a Value e.g. if the successor of your LoadFunction was an IndirectCall. Neat.

Call still takes Function and does type-apply itself? Or, how about we continue the same line and make Call take a Value input so could be applied only to (1) a Const or (2) a TypeApply of a FuncDefn?

@doug-q
Copy link
Collaborator

doug-q commented Oct 23, 2024

Call still takes Function and does type-apply itself? Or, how about we continue the same line and make Call take a Value input so could be applied only to (1) a Const or (2) a TypeApply of a FuncDefn?

I think you mean "Call take a Const input". Yes you could do this, with the advantage that now only TypeApply applies type args to functions. You could also leave Call as is for convenience, and add CallConst that takes a Const edge.

@acl-cqc
Copy link
Contributor

acl-cqc commented Oct 23, 2024

Yes, Call takes Const not Function, sorry!

@ss2165
Copy link
Member Author

ss2165 commented Nov 4, 2024

With the feature as described it is not allowed for extension operations to take Function edges from FuncDefn/FuncDecl

This is the case in the PR linked above. I like the change you propose but I think that is a follow up feature so should be in a new issue.

@ss2165
Copy link
Member Author

ss2165 commented Nov 20, 2024

Moved to hugr "v2" model work #1433

@ss2165 ss2165 closed this as not planned Won't fix, can't repro, duplicate, stale Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants