Support Analog DontCare bulk-connect

[ducky64]

Exactly what it says on the tin.

Root cause is that Analog and DontCare are both subtypes of Element, so it takes the Element <> Element path in BiConnect.connect, it makes stronger assumptions about Element than Analog and DontCare provides (particularly, assumes Element is unidirectional). Improving the type system so that these guarantees can be made more explicit (eg UnidirectionalElement or something, or matching on Bits subtypes) might allow the type checker to categorically detect these kinds of bugs, but that's something for discussion and/or a future PR.

Actual behavior (in Chisel; FIRRTL may do more checks) with regards to DontCare (and also Analog in general) seems to be an inconsistent mess, and this PR only slightly patches up things. Here are the cases:

• Analog <> Analog: each Analog is checked to make sure it isn't part of multiple BiConnects. Implementation looks like a bit of a hack, Analog stores biConnectLocs. Perhaps this should be delegated to FIRRTL?
• Analog <> DontCare: this PR adds the no-multiple-BiConnect check to be consistent with BiConnect.
• Analog := DontCare: currently legal, but does not contribute to the no-multiple-BiConnect check (so it's inconsistent - this this appears to be an edge case that also got lumped on the Element := Element path)). Should we even allow Analog as part of MonoConnect?
• attach(Analog*): there is no check against an Analog being part of multiple attach operations. I think you can BiConnect an Analog and also have it be part of one or more attach statements.

Perhaps not completely unexpected that another bugfix went deep into the rabbit hole...

---

Related issue: Fixes #882

Type of change: bug fix

Impact: API addition (no impact on existing code)

Development Phase: implementation

Release Notes
Allow Analog <> DontCare, DontCare <> Analog

[edwardcwang]

Generally looks good. Thanks Richard for looking into this. A few comments/questions:

1. What's the deal with last-connect semantics on <>? e.g. a := 1.U; a := 2.U is the same as a := 2.U, but about foo <> x; foo <> y?

2. We definitely want to support multiple/arbitrary attach/connections with Analog. Food for thought: should attach(foo, x); attach(foo, y) be treated similarly/identically to foo <> x; foo <> y where foo, x, and y are all Analog?

[ducky64]

I'm not sure why <> behaves differently from attach. Does anyone know what was the reason behind checks for multiple <> to a single Analog, but not for attach?

It looks like biConnectLocs was introduced in #422 by @jackkoenig, though skimming the discussion I don't see any explicit rationale for the differing behavior, other than maybe avoiding needing to deal with last-connect semantics on <> whereas no such semantics have been defined for attach?

But definitely worth a discussion on whether we can unify behavior.

[jackkoenig]

Semantics currently (ignoring DontCare):

• <> works with Analog elements, but only once (because unlike normal last connect semantics, Analog can be attached to multiple things)
• You can attach multiple times, so attach(foo, x); attach(foo, y) is the same as attach(foo, x, y)

What should semantics be?

• Can you := with Analog on left-hand side and DontCare on the right hand side
• Should <> last connect semantics apply to Analog? How does it related to attach?

[edwardcwang]

this PR by itself seems OK to me, can always clarify semantics in future work

[ducky64]

Resolutions from today's dev meeting:

• The motivation behind the one-<> limit is that <> has last-connect semantics, but Analog users might expect attach semantics. Ideally, users would know exactly what they're doing, but since most users aren't psychic, the next best solution is just to eliminate the confusion. So it's kind of a least-worst solution. But in the future, we can always expand the API if desired (it's strictly an API expansion).
• We should improve the multiple-<> error to mention attach, and link to the wiki and/or a relevant issue.
• It's unclear whether we want to allow mono-connect DontCare to Analog
  • It currently is used, so if we plan to remove it, it should be through a deprecation
  • Currently, we can mono-connect DontCare to both inputs and bidirectional data. It's also currently unclear what all the connect semantics currently are. Probably not a great place to be.
  • I'm going to create a table of all current connect semantics, from the MonoConnect and BiConnect giant match statements. We can then figure out what we want semantics to be based on that, and given any identified unintentional cases.
  • We may want to discuss the practical effect of connecting DontCare to Analog, and also edge cases of combinations of attach and <> Analog to DontCare

[ducky64]

MonoConnect rules

• Element subtype connects: does a directionality check and module context check, then issues the connect
  • (Bool | UInt | SInt) := (Bool | UInt | SInt)
FixedPoint := FixedPoint
Clock := Clock
EnumType := (UnsafeEnum | EnumType)
UnsafeEnum := UInt
• Aggregate type connects: performed recursively elementwise. No type equivalence check appears to be done
  • Vec := Vec
    Vecs must be of the same length, but not type checked at this level.
  • Record := Record
    Source may have more field than sink, but unless connectFieldsMustMatch compile option is enabled, source must have at least all of sink's fields, by string field access (instead of type comparison)
  • Record := DontCare
Vec := DontCare
    handled with recursive connect DontCare
• (anything) := DontCare
DontCare := (anything)
• (any other connect not listed)
  illegal

BiConnect rules

• Analog <> Analog
  analog attach
• Element <> Element
  Note: element includes Analog, DontCare, Bits
  does a directionality check and module context check to determine source / sink, then issues the connect
• Vec <> Vec
  Vecs must be of the same length, but not type checked at this level. Performed recursively.
• Vec <> DontCare
DontCare <> Vec
  Performed recursively
• Record <> Record
  checks both records have the same fields, by string field access (instead of type comparison)
• Record <> DontCare
DontCare <> Record
  Performed recursively
• (any other connect not listed)
  illegal

Other notes

• There's definitely some code deduplication to be done with descending into aggregate types.
• It might be worth breaking out the Element connect (or really more accurately, Bits and Clock; those cases should not handle DontCare and Analog) connect code into a method shared between MonoConnect and BiConnect.

[ducky64]

Resolution: add test cases (@ucbjrl) of current connect semantics, then we can discuss what Analog DontCare should do.

Fun fact: you can UInt <> Clock in Chisel, though that gets caught in FIRRTL

[ducky64]

While we're talking about connect semantics, another edge case to consider: should myUIntWire <> 42.U be legal? What about myBundleWire <> myBundleLiteral? Both are currently not, because bulkConnect can't determine sink / source because both wires are internal.

[ucbjrl]

I believe this is good to go. API refinement and internal refactoring can wait fora future release.

[ducky64]

Should VerifyConnectionProperties be in a separate PR?

[ducky64]

Resolution from today's meeting: merge Analog changes as an immediate-term patch, the test should be its own separate PR