Allow local constants

[ss2165]

@aborgna-q
27 April 2023

We need a separate category of inter-graph edges for const wires, from a const value node to a load operation.

@ss2165
27 April 2023

Do we? Or do we just say “inter-graph edges can have Value or Const kind”?

@aborgna-q
27 April 2023

Inter-graph const edges don’t work with the requirements of external nor dominator edges.
They don’t really have any special requirement since the source node is always in the top-level module and completely disjoint from the constant load node.

@ss2165
27 April 2023

I’m not sure we want to enforce that const nodes are always at the top level

@aborgna-q
28 April 2023

Uhm. Where else should they be allowed to be? I think currently they can only be children of the single root module?

@ss2165
28 April 2023

I’m suggesting that const and def/declare are allowed to be in any dataflow context and obey the same locality rules as Value (Ext or Dom)

This allows these definitions to be scoped rather than accessible from anywhere in the HUGR

@aborgna-q
2 May 2023

That sounds nice, but I don’t think the def/declare nodes should need to care about state ordering.

The validation should be a bit more lax than the Dom/Ext edges, requiring a common parent but no special ordering between siblings.

Btw, can we define mutually recursive operations?

@ss2165
2 May 2023

yes, mutual recursion should be possible

@acl-cqc
3 May 2023

I think “Ext” edges work if the constants are module-level; they require that the parent of the source is an ancestor of the target. The source is a constant, so its parent is the module root, which is ancestor of anything? This probably is the right constraint for more local constants too but I am not yet convinced about those.

@acl-cqc
16 May 2023

I’ve changed this so that it says it applies to value edges only. That is, this section says nothing about ConstE edges. I think the section introducing ConstE edges says enough about them, but maybe we need more (somewhere)? Otherwise, can we resolve this comment now?

@cqc-alec
16 May 2023

So ConstE intergraph edges wouldn’t require the Order edge, is that right?

👍
1

@acl-cqc
16 May 2023

Indeed.

[acl-cqc]

Do I understand correctly that the questions wrt. a new "locality" are only, what do we require in terms of locality to enable local constants?

The main issue I see here is that this makes locality depend upon node type, and asymmetrically at that. Previously localities had no concern with the node type, nor port type (ports did not specify a locality). However, the proposed "const locality" makes new kinds of requirement on the source (namely, that they be a const node) - whereas the target will be a value port of unspecified locality.

Another way of looking at this might be that the target port, if the incoming edge is of "const locality", now has to do extra work (loading the constant from the constant pool into memory, say - rather than it already being in a register).

[ss2165]

Do you think it might be better then to say instead

Edges leaving const/def nodes may have local or ext locality, but the state edge requirement can be elided/dropped since they are statically defined and so always causally precede the target.

[acl-cqc]

Yes, I like that! :-) In fact, one might go further and allow 'dom' locality too; why not? Then const node outports are like any other outports (specified by a signature which talks about type but not locality).
The "special rule" is then that const nodes need not have the order edges for "ext" locality. We should think here whether we want Const nodes reachable via Order edges from the Input, too. A different "special rule" would be that you can skip the order edges (for ext locality) if the source's parent is the module root; I kinda like that because it looks only at the hierarchy, not the type of node...

[ss2165]

Currently we use ConstE edges for const/load_const as well as def/declare/call. If we combine const/load_const in to one and have Value kinded edges leaving them, it is unclear what to do with call.

A somewhat extreme simplification is to say call also just has a Value input (essentially, remove call and rename indirect_call -> call) and have the static function definition be something that is structurally inferable from the HUGR connectivity (i.e., this is a static call because the source is a def), rather than something locally enforced by signature

[aborgna-q]

I'd prefer keeping the ConstE here, renaming it to StaticE.
Distinguishing between dynamic and static definitions based solely on the source may be a bit finicky.

[acl-cqc]

I agree it's probably important to distinguish direct vs indirect calls.

Do we really need to be able to "direct call" a Graph stored in a Const? Can we make call use defs only? Sadly I'm not sure that helps as we want local defs too, right....

[acl-cqc]

Maybe we need to keep ConstE edges, still as going between const/def -> load_const/call, as before. But, we

1. make load_const able to have any number of outputs....trivial given No explicit copy #80
2. Add a new "local_const" node that has no ConstE input, and contains the constant. But has value outputs (like load_const) still.

Being able to compositionally add new (local_)constants as part of a subgraph seems important, but we might also want to be able to "share" identical constants that were added, in multiple places, in this way, i.e. by combining them into a single const + multiple load_consts.

I mean, it does seem odd to have a new-style "const" node with value edges that enter several distinct functions from outside. Perhaps ConstE is a good distinction to have.

[acl-cqc]

Third option. Do we really need to be able to call functions in const nodes? Can we not just have them call defs only? Then we keep a separate class of edge, I definitely prefer to call them Static in this case, just for call -> def ?

[ss2165]

I agree with the discussion above. On balance I think I would like to go for:

1. Rename ConstE -> Static (and references to const_input etc. to static_input in the code)
2. Static edges go from def/const to call/load as now
3. The new "local_const" node behaviour is useful and can probably be captured by building/viewing APIs without a new node.

[cqc-alec]

I guess we can also rename ConstN to Const, which is a bit nicer, as there won't be any possibility of confusion.

[ss2165]

In the spec currently we state the Value output of LoadConstant can have any locality - I think we can make Const and LoadConstant more separate by requiring a Local locality on the outputs.

[ss2165]

Partially implemented in #112. The builder now allows adding constants at any level. This required a horrible hack to "NotADag" validation to add logic for "when top sorting through nodes, if node is a LoadConstant connected to a Const with the same parent, count it as two nodes instead of 1".

When defs are also made local this will require the same special casing.

[ss2165]

Now implemented with the special casing described above.

#163