[FIRRTL][LowerAnnotations] Fix non-probe type compat check. #6822
Conversation
| !areTypesEquivalent(sourceFType, sinkFType)) { | ||
| !areTypesEquivalent(sinkFType, sourceFType)) { |
There was a problem hiding this comment.
Type equivalence is not commutative? This seems like an issue with the type equivalence check?
There was a problem hiding this comment.
LOL yeah it's not symmetric and therefore also not an equivalence relation, possibly for other reasons I'm forgetting. This is @mmaloney-sf's favorite part about it 🙃 😉 .
There was a problem hiding this comment.
To be clear this is the FIRRTL spec's notion of "type equivalence", I don't think it's bugged (not for lack of being an equivalence anway) but at a very-quick glance it doesn't mention const there presently, although it's still not symmetric (Reset can connect to UInt, can't connect UInt to Reset).
There was a problem hiding this comment.
Good point about resets which are obviously not commutative.
I think the problem here is more that the spec is presenting this as an "equivalence" when it's really "verification of connection source and destination". It's not inherently wrong that equality isn't commutative, it's just weird.
There was a problem hiding this comment.
Right, shouldn't be calling its connectivity rules (or whatever) equivalence or equality probably as that's misleading if nothing else. I haven't checked but can't help but wonder if it was originally a proper equivalence and then drifted or something. Welp.
Anyway, mind approving the PR? 👍
There was a problem hiding this comment.
I think it was before abstract reset. The spec was always written as something like "the source and destination must be equivalent types (see type equivalence section for a definition)." Notably, I don't think that "type equivalence" actually mattered except for connects. I.e., this was always that aforementioned connect verification. We should probably just roll it into the connect section.
Add test as "legacy wiring" as non-ref-type-port path is a hidden option.
Compatibility dest/source operands were backwards, introduced in #4656 .
Fixes #6819.