[RFC] Add `func::GraphFuncOp` which holds a graph region

Over in CIRCT, we have operations that reimplement func::FuncOp except that they hold a graph region instead of an SSACFG region (e.g., handshake::FuncOp: circt/HandshakeOps.td at main · llvm/circt · GitHub). Additionally, the custom FuncOp requires a new terminator and call/instance operation, which leads to more copy-pasting from the func dialect.

As the func dialect is not necessarily limited to SSACFG regions, we suggest to add a second FuncOp, e.g., GraphFuncOp, to the func dialect. Such a new operation would be almost the same as a FuncOp except that it marks its region’s kind as Graph.

1 Like

Have you considered adding an attribute to the existing func instead?

As far as I could tell, the region kind property is static. Thus, the region of func will always be a SSACFG kind.

1 Like

I don’t have a strong opinion on this, but when we added ml_program.subgraph there were strong opinions on naming the op and the terminator in a domain specific way.

Were you wanting to use this as a replacement for the CIRCT handshake func op? I notice that has a HasClocks trait. Are you sure a fully generic version of the op is appropriate? It looks like there are also other domain specific constraints? (Last argument is a “control” and returns have a single use) I don’t know this part of CIRCT very well – just noting that these are the kind of things that are reasons why you want your own.

Yeah, we consider replacing handshake.func with a more generic graph region function.

We are currently trying to make CIRCT’s handshake dialect more generic, as it is very much dependent on its original use case. The “control” requirement and the single-use were already dropped (we might have forgotten the doc change…). Furthermore, the traits are only relevant for the lowerings, which indicates that they should be dropped as well.

Assuming that all this will be changed, the handshake.func operation becomes a generic graph region function.

The general direction we took with FuncOp is to move more and more of its functionality to an interface, so that dialects can define their own FuncOp.
From this point of view, what is the benefit of adding a func.graph operation instead of letting anyone create their own as needed?

There is quite a lot of additional functionality in the func.func op we would have to re-implement. Furthermore, both call, as well as, return are limited to func, thus those have to be duplicated as well.
While a partial implementation obviously works, it still has the potential to diverge from MLIR at some point.

Hence my original question about extending the existing op instead :wink:

So you are suggesting that func.call will be able to call func.graph_func? I guess what I am missing in this proposal is how this new op connects to the other dialects or flows. We have an admittedly implicit assumption that the func dialects is something that describes code ultimately running on CPU/GPU/accelerators. It’s unclear how a graph-region function would fit in there. Now, it is not a stated goal of the dialect and therefore isn’t something that we are expected to uphold as long as there is some benefit for MLIR itself. So far, it feels like just reducing the amount of code downstream in CIRCT.

What exactly? The goal with the interface was to make it easier. If some more code refactoring is needed to make it more convenient we should do it as well.
Looking at the ml_program.func and ml_program.subgraph definition it seems fairly reasonable.

If we add a new op in the func dialect, that wouldn’t change the status of call and return though.

What exactly? The goal with the interface was to make it easier. If some more code refactoring is needed to make it more convenient we should do it as well.
Looking at the ml_program.func and ml_program.subgraph definition it seems fairly reasonable.

All the builders, create functions and cloning are very handy. Reimplementing everything when the only difference between func.func and handshake.func is the region kind seems unnecessary. Unfortunately, there doesn’t seem a nice way of reusing all these implementations without duplicating things in MLIR directly. If that’s indeed the case, we can just keep it in CIRCT as is, I guess.