[FIRRTL][ExpandWhens] Allow zero-width wires to be uninitialized

[albertchen-sifive]

Don't emit uninitialized errors for zero-width wires

[seldridge]

LGTM, thanks!

Feel free to land after addressing review.

[seldridge]

Comment nit (PR body/commit nit): this is really any zero-width type, not just wires. This does avoid initialization checks for ports, registers, etc., yes?

Mega-comment nit: Capitalization/end sentences with a period. 😉

[seldridge]

Nit: the PR can skip doing any checks here. It's successful if there's no error.

[uenoku]

Though I agree it's useful to do this but I'm not sure it's generally good to create an exception for strong guarantees of initialization requirements for wires, e.g. we might expect getSingleConnectUserOf returns non-null after ExpandWhens. Maybe alternatively is it reasonable for ExpandWhens to create connections for uninitialized zero-width wires?

[dtzSiFive]

If this change should be made, it should be reflected in the FIRRTL spec's "Initialization Coverage" section which presently does not have exceptions for zero-width components (wires or otherwise).

Injecting a driver from a constant does seem preferable (re:pipeline assumptions and general clarity/goodness), but I'm not sure this change should be made.

Can you motivate this a bit more?

[albertchen-sifive]

I'm not too familiar with the exact use cases, but I presume it is a quality-of-life change for chisel users to make it easier for them to handle zero-width cases in their generators.

[jackkoenig]

@dtzSiFive Agreed that this should be on the spec as well.

Injecting a driver from a constant does seem preferable (re:pipeline assumptions and general clarity/goodness), but I'm not sure this change should be made.

Can you motivate this a bit more?

@mmaloney-sf, @seldridge, and I discussed this with a few weeks ago offline. The basic idea here is to make the initialization requirements consistent between empty bundles and zero-width types. Empty bundles are not required to be initialized and it made sense that other "empty" types shouldn't need to be either.

From the generator perspective (and I think similar things would apply to parameterized FIRRTL), when the parameterization gives you the degenerate case of an empty or zero-width type, there are many operations that become illegal to apply so you end up having to conditionalize that code on whatever parameter results in the empty or zero-width type. Connections are often included in this conditional code block and are thus elided in the empty or zero-width case. It ends up being a pain to add an additional case of initialization for the empty or zero-width typed components. Note that trying to initialize the generator types that lead to empty Bundles would be tricky because in the empty case, there are no fields to dot into and you end up having to special case that logic (if it were required).

Consistency makes sense here and IMO consistency on the side of not requiring initialization best serves the user.

[seldridge]

Thinking about the parameterized case... that is really something else. E.g., if you have something like the following with completely made-up syntax where <x: Int> is a parameter to module Bar:

circuit Foo:
  module Bar<x: Int>:
    output a: UInt<x>
  module Foo:
    inst bar of Bar<x=0>

a has to be initialized in all situations, arguably even if x=0 as set outside the module. (Parameter constant propagation shouldn't be required for correctness.)

What this PR is doing is bringing the following three representations in line:

circuit Foo:
  module Foo:
    output a: UInt<0>
    output b: {}
    output c: UInt<1>[0]

Currently a is an error and b and c are not (in both the MFC and SFC---the MFC got this behavior from the SFC). This PR brings these three in line such that any zero-width type does not have to be initialized. (Zero-width type is not defined.)

However, this really does need some clarity in the FIRRTL spec as the Initialization Coverage section is clearly lacking.

[jackkoenig]

a has to be initialized in all situations, arguably even if x=0 as set outside the module. (Parameter constant propagation shouldn't be required for correctness.)

Not necessarily, it's very likely some of the firrtl for the x=0 case will be guarded by some parameter time if clause, just like it often is in Chisel.

[albertchen-sifive]

firrtl-spec PR: chipsalliance/firrtl-spec#156

[dtzSiFive]

Thanks all for the comments and context, and thanks for the spec PR!

the degenerate case of an empty or zero-width type, there are many operations that become illegal to apply so you end up having to conditionalize that code on whatever parameter results in the empty or zero-width type. Connections are often included in this conditional code block and are thus elided in the empty or zero-width case. It ends up being a pain to add an additional case of initialization for the empty or zero-width typed components.

Interesting! I'll take your word for it, but two things FWIW:

1. An empty aggregate (bundle/vec) is not the same as a signal of zero-width. You don't initialize them for the same reason initializing all members of a bundle doesn't ALSO require initializing the bundle itself as such. As an additional consideration/differentiation, this change also means it's okay to not initialize a wire if it happens to infer to zero-width which feels very weird.

2. I was under the somewhat vague impression one big reason we had zero-width wires at all was because folks writing generators/parameterization/etc /didn't/ want to specialize the degenerate width=0 case and so it was better to elide that for them afterwards. If you're clear on this, I wonder why it's natural to avoid the initialization when width is zero but not avoid the signal entirely?

[dtzSiFive]

Consistency makes sense here and IMO consistency on the side of not requiring initialization best serves the user.

Consistency across things that have zero-width, you mean? I'm not sure I agree this is overall more consistent or why that's the framing over which consistency is measured. This makes initialization of UInt's inconsistent based on their width, for example.

If in FIRRTL you initialized the individual bits of a UInt and thereby implicitly initialized the entire thing, I think this would be making it more consistent.

I'm also wondering if this means you can drive a zero-width signal on one branch but not another and that's, "fine" actually? Looks like currently no, but with this PR "yes" (this also answers the question "how does a signal infer to zero-width without being driven?"):

circuit ZWCond:
  module ZWCond:
    input c : UInt<1>

    wire w : UInt
    when c:
      w <= UInt<0>(0)

To be fair apparently you can do this for empty-length aggregates to so maybe everything is weird:

circuit EmptyVecCond:
  module EmptyVecCond:
    input c : UInt<1>
    input vin : UInt<1>[0]

    wire v : UInt<1>[0]
    when c:
      v <= vin

[jackkoenig]

this change also means it's okay to not initialize a wire if it happens to infer to zero-width which feels very weird.

If you think about width inference as type inference I don't think it's that weird.

was under the somewhat vague impression one big reason we had zero-width wires at all was because folks writing generators/parameterization/etc /didn't/ want to specialize the degenerate width=0 case and so it was better to elide that for them afterwards. If you're clear on this, I wonder why it's natural to avoid the initialization when width is zero but not avoid the signal entirely?

You are correct that that's a reason for zero-width wires and the motivation is the same here. You need zero-width wires to work as expected under all operations to avoid the need to special case. For example avoiding special casing requires reductions (or, xor, and) to be defined, use as a dynamic index to be legal, connecting to and from zero-width wires to be legal. All of these are the case, so now it's just a question of the legality of not driving a zero-width wire.

Taking a step back and thinking about it from first principles. What is the purpose of initialization checking? The goal is to avoid accidental undefined behavior (forcing the user to opt-in via the invalidation operation). Zero-width wires (and empty aggregates) do not have any dynamic behavior, there is only 1 value they can possibility represent so there is no undefined behavior to guard against.

I'm also wondering if this means you can drive a zero-width signal on one branch but not another and that's, "fine" actually? Looks like currently no, but with this PR "yes" (this also answers the question "how does a signal infer to zero-width without being driven?"):

To be fair apparently you can do this for empty-length aggregates to so maybe everything is weird

For empty aggregates it's even more important. With a Vec, you often iterate on the elements to assign things. If the size is 0, there's nothing to iterate on so no connection is emitted. Obviously that's not something that happens with UInt since we don't have subword assignment, but similar sorts of things can where you iterate on some collection to drive your UInts and when the UInts are size 0, the collection is empty.

[darthscsi]

Arguably the historic reason one didn't have to initialize an empty aggregate was there was no way of writing aggregate literals.

Why not instead solve this problem by requiring all things to be initialized instead of exempting more things from the rule?

[jackkoenig]

Arguably the historic reason one didn't have to initialize an empty aggregate was there was no way of writing aggregate literals.

While applies to some cases, it doesn't apply to all.

It is a very common coding pattern in Chisel to iterate on the elements of a Vec and connect each member in the loop. In many of these cases, you don't need to "initialize" the Vec because you're connecting to each member in the 1 place you want to. But in cases where the parameterization may result in the size of the Vec being zero, you'd then have to special case initialize the Vec when size == 0 even though there is no need to do it for any other parameterization. For example:

myVec.zipWithIndex.foreach { case (elt, idx) =>
  // Note it's common to have lots of other connections in here too
  elt := somethingElse(idx) + coolFunction(idx, ...)
}

Requiring size 0 Vecs to be initialized would require us to add a special case initialization:

if (myVec.isEmpty) {
  myVec := DontCare
}

The issue here is what is the point? The purpose of initialization checking is to remove a class of undefined behavior. There is no undefined behavior for empty Aggregates nor zero-width wires. There are no bugs being caught by requiring initialization here, instead there's just unnecessary boilerplate.

Worse, Chisel users will be encouraged to just blanket DontCare the Vec rather than doing the precise special case I have above, they'll probably just add myVec := DontCare where the Vec is declared. This makes the code worse to maintain because if they refactor the code and forget to connect the Vec under some when condition, they've waived initialization checking with that DontCare so they wont be warned by the compiler--instead they'll just have to find the bug in simulation.

TLDR Requiring initialization checking for empty aggregates (especially empty Vecs) provides no actual benefit while forcing the user to special-case their Chisel in some circumstances.

[darthscsi]

So backing out from the empty aggregate issue, uninitialized things (kind of: https://github.com/chipsalliance/firrtl-spec/blob/main/spec.md#invalidates) generally have indeterminate values (https://github.com/chipsalliance/firrtl-spec/blob/main/spec.md#indeterminate-values). There are plenty of problems right now with code making assumptions about the implementation of indeterminate values. This is trying to make the corner case of knowing that there is only one possible value the same as saying the thing has that value. This adds irregularity to the IR and complicates the safety checks in the code. Neither of these is desirable.

When is this going to come up? If I don't know the width of something because it's either uninferred or it's based on a scala value, then in general I'm going to have to initialize the value to something. Saying there is an exception if it is zero would require writing special-cased code to deal with it.

[darthscsi]

I would much rather be consistent in the handling of the integer types.

IR should be consistent and strive towards fewer special cases. Having exactly 2 integer types which are special cased here (and possibly other places when this is tried at scale) to avoid an invalidate is very much a special case.