[FIRRTL] Add a new op interface for combinational loop detection.

[prithayan]

Add a new CombDataflow op interface to FIRRTL.
This interface is used for specifying the combinational dataflow that exists
in the results and operands of an operation. Any operation that doesn't
implement this interface is assumed to have a combinational dependence
from each operand to each result.

Only FIRRTL register and memory ops implement this interface, but it can be used in other
dialects that intend to use the CheckCombCycles pass.

[uenoku]

Is this interface flexible enough to model dependency for probe?

[prithayan]

I'm not sure how to handle Reftypes, since it cannot be determined locally, it requires a lot of state information like, "What the Ref type is pointing to ?", "What are the aliases for FieldRefs". I don't think this interface can handle refs.

[uenoku]

I'm not sure how to handle Reftypes, since it cannot be determined locally, it requires a lot of state information like, "What the Ref type is pointing to ?", "What are the aliases for FieldRefs". I don't think this interface can handle refs.

That's fair. LGTM.

[dtzSiFive]

The pairs of dataflow between FieldRef's are required to be leaf-most/ground types, is that right? This would be good to note.

Having interface for this is great! I have some doubts about default being assuming the operation is indeed "combinatorial" (since that means mixing in operations we don't understand will cause this pass to /error out/ until those operations implement this interface to opt-out appropriately)-- that is to say, from a "safety" perspective it's conservatively more correct to over-record the dataflow (well, from operands -> results at least, it may have more complicated behavior re:side-effects beyond flowing to its results locally) through unknown operations but from a "don't abort the compilation if we really don't know" perspective it seems like maybe default should be to ignore unknown operations? If Dialects wish to participate in /FIRRTL/'s notion of comb-loop-checking, they can opt-in.

Just some thoughts, probably this serves our needs for now without overdoing it.

Implementing it for our operations is a nice way to show this working, neat! Looking at this a bit, it seems for registers this interface only tells part of the story -- looks like the pass still hardcodes knowledge of those operations (reg/regreset) -- can this be refactored? (If a new op wants to be register-like, can it use this interface to accomplish that?)

[prithayan]

@dtzSiFive , thanks for your comments,

The pairs of dataflow between FieldRef's are required to be leaf-most/ground types, is that right? This would be good to note.

Right, I will add a comment with this note.

Having interface for this is great! I have some doubts about default being assuming the operation is indeed "combinatorial" (since that means mixing in operations we don't understand will cause this pass to /error out/ until those operations implement this interface to opt-out appropriately)-- that is to say, from a "safety" perspective it's conservatively more correct to over-record the dataflow (well, from operands -> results at least, it may have more complicated behavior re:side-effects beyond flowing to its results locally) through unknown operations but from a "don't abort the compilation if we really don't know" perspective it seems like maybe default should be to ignore unknown operations? If Dialects wish to participate in /FIRRTL/'s notion of comb-loop-checking, they can opt-in.

Actually, here I took the shortcut, that is the assumption is that every operation, other than just the 3 in FIRRTL, have the default behavior. So, you should need to annotate only the very few special ops. That's why initially I was suggesting a Sequential interface, that denotes just the ops that should be excluded from comb loop checks.
I agree with you here, any non-FIRRTL dialect that wishes to participate has to explicitly opt-in, again the assumption is that very few ops will have a special combinational dataflow, so fewer updates are required. It is the responsibility of the client to ensure special ops are skipped and false loops are not reported.

Just some thoughts, probably this serves our needs for now without overdoing it.

Implementing it for our operations is a nice way to show this working, neat! Looking at this a bit, it seems for registers this interface only tells part of the story -- looks like the pass still hardcodes knowledge of those operations (reg/regreset) -- can this be refactored? (If a new op wants to be register-like, can it use this interface to accomplish that?)

That's a good idea, so you mean a new RegisterLike op interface ?