[FIRRTL] Warn on abstract resets on extmodule's.

[dtzSiFive]

When parsing FIRRTL >= 4.0.0, check and warn on use of abstract reset on extmodule's. This should be an error.

chipsalliance/firrtl-spec#181

Previously implemented as part of #6731, but used in practice so deferred. Let's revisit now.

Based on #7248, converted to warning.
Start by emitting warnings, not errors.

[seldridge]

How many warnings does this generate on internal designs?

[dtzSiFive]

Thanks for taking a look! I'm having cold feet on this, and am unsure the FIRRTL spec change is 100% right. More below if you're interested in this line of thinking/musings.

How many warnings does this generate on internal designs?

Let's find out! 😉

Just kidding, already built a smaller internal core and have a PR I was planning to open shortly to fix the handful of cases that ran into.

Practically I don't know on larger cores, but it's rather limited as its only once per static location in the FIRRTL. I'd say dozens across our codebase's generators, maybe amplified in some cases.

I'm staring at this, because things are a bit subtle when we start requiring concrete.

FIRRTL<-->FIRRTL (extmodule is implemented by a FIRRTL module), especially with us already enforcing public-side concrete resets, seems "right" for both input and output ports. It seems inconsistent as-is and intrinsically good to enforce this same constraint on the extmodule side as well.
Having disagreement here suggests something strange/wrong and it's not something we'd support as-is if NOT separately compiled.

Furthermore it smells of "you tell ME which kind!" which in FIRRTL suggests "my registers will be emitted to match!" which is of course not the case.

Reset kind as a parameter gets the mechanism to in theory handle this but I'm not sure that's a reasonable thing to parameterize in terms of the generated SV (such that we advertise from a single compilation to let you select either): at what point do you really have two entirely different circuits and should just embrace that?

You can always use casts to break inference chains if the desire is to allow one side to be sync OR async (and to error if not constrained by other sources), such as an extmodule with input port indicating its reset is async and we want to support our design being sync or async.

I ran with some enumeration earlier today where output ports should be concrete (output/input crossed with FIRRTL/non-FIRRTL on each side), which still feels right but I don't like how it all sits just yet, to be honest. FWIW abstract extmodule output port is the best (AFAICT) way to generate a driver-side-unconstrained reset network and this closes it, I thought there was another way but haven't conjured it yet this time around.

The rule in the spec is unconditional, but looking for the motivation I'm not sure I find it there.

What is better as a result of this change? 🤔

[circt-bot]

Results of circt-tests run for dd868fc compared to results for cc142fe: no change to test results.

[seldridge]

I think alignment of FIRRTL public module with external module is sufficient motivation for this.

Alternative reasoning: the spec says this is what it is! So we should either change firtool or change the spec. 😉

Yes, good point about this being difficult to actually show a cascade of warnings because it is per-extmodule.

LGTM.

Can you spawn off tickets to fix the internal external modules which don't conform and assign them to owners to fix?