llvm / circt

Circuit IR Compilers and Tools
https://circt.org
Other
1.67k stars 298 forks source link

[SV] Combine RegOp/LogicOp -> `sv.var` #6318

Open dtzSiFive opened 1 year ago

dtzSiFive commented 1 year ago

We currently have two ops that behave differently (and are interpreted/handled differently), sv.reg and sv.logic.

Both claim to "Declare a SystemVerilog Variable Declaration of 'reg' type." (or "logic"), but these are explicitly the same thing:

From 6.11 of 1800-2012 (p68):

The keyword reg does not always accurately describe user intent, as it could be perceived to imply a hardware register. The keyword logic is a more descriptive term. logic and reg denote the same type.

Presently looks like our logic is kinda treated like a wire (see #6317), and is not handled as RegOp everywhere.

There's a concrete reason we added this, so let's make sure those needs are still met (something about automatic logic I believe). Hopefully we can just collapse the two :crossed_fingers: .

Proposal (h/t to @fabianschuiki for good parts of this :wink: ):

sv.var -- single op for variables.

Both this and sv.wire would optionally take RHS operand for declaration-time initialization -- for wires this is continuous and for variables this is one-time. For net-type declarations (sv.wire) we can optimize continuous assignments into the sv.wire by adding the assignment RHS as an operand to accomplish what's done during emission presently.


I'd suggest we emit sv.var as logic instead of reg for basic four-state integer values, and continue with T x for other types (vector/struct), although we could include the var keyword to be explicit.

This would lose the ability to produce reg, but this could be an emission option if becomes an issue. LMK if you have any insights or concerns about tools or compatibility by this sort of change.

seldridge commented 1 year ago

Generally agree with all this. This is slightly divergent from the SV rationale, though I think in a reasonable way.

What I'm getting at is SV is very much a syntactic dialect. It's more about structuring things to emit Verilog as opposed to capturing the semantics of it. Semantically, Verilog has two basic kinds of types: nets and variables. Collapsing reg/logic together into a variable seems fine (as that is what they are!). This is a marginal move towards a more semantic representation of Verilog. The only place this is problematic is if a user desperately wants to mixed emission of registers and logic. This can be allowed with some (mandatory) attribute.

mortbopet commented 1 year ago

Pinging @blakep-msft to check that this would also work for what we need to emit for Ibis.

blakep-msft commented 1 year ago

I think this should be fine for Ibis.