deprecate WireInit

[sequencer]

WireInit and WireDefault stayed together for almost 3 years, see #959
It's reasonable to deprecate one of which and remove WireInit in the next Chisel major version.

Contributor Checklist

• Did you add Scaladoc to every public function/method?
• Did you add at least one test demonstrating the PR?
• Did you delete any extraneous printlns/debugging code?
• Did you specify the type of improvement?
• Did you add appropriate documentation in docs/src?
• Did you state the API impact?
• Did you specify the code generation impact?
• Did you request a desired merge strategy?
• Did you add text to be included in the Release Notes for this change?

Type of Improvement

• code cleanup

API Impact

Backend Code Generation Impact

Desired Merge Strategy

Release Notes

deprecate WireInit

Reviewer Checklist (only modified by reviewer)

• Did you add the appropriate labels?
• Did you mark the proper milestone (Bug fix: 3.4.x, [small] API extension: 3.5.x, API modification or big change: 3.6.0)?
• Did you review?
• Did you check whether all relevant Contributor checkboxes have been checked?
• Did you do one of the following when ready to merge:
  • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
  • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

[ekiwi]

How does this relate to VecInit?

[jerryz123]

What is the incentive for removing WireInit? Many existing projects use WireInit extensively.

[aswaterman]

In a SiFive repo, both WireInit and WireDefault are used extensively, but WireInit is by far the more popular choice.

[jerryz123]

In a SiFive repo, both WireInit and WireDefault are used extensively, but WireInit is by far the more popular choice.

Now I feel less bad about using WireInit exclusively

[mwachs5]

i feel like we bikeshedded this already, but we already have all the *Init why introduce a new word Default?

[mwachs5]

i have no problem if we agree WireDefault is the right name to globally find and replace

[jerryz123]

I don't want to debate whether WireInit or WireDefault is the better name, it seems WireDefault will be the recommended API going forwards, and that's fine.

I just want to point out that maintaining the WireInit API is zero burden, so I don't see the motivation for removing it (or adding the deprecation flag).

[seldridge]

I just want to point out that maintaining the WireInit API is zero burden, so I don't see the motivation for removing it (or adding the deprecation flag).

It's not zero burden for Chisel in testing, for onboarding Chisel users, and readability of existing Chisel libraries, though. If there's one function for creating a wire with an initial (or default) value that seems like a good language design principle to me. I hope that we can all agree that having one way of doing this would benefit Chisel as a language, even if it requires users to update their code.

Analogously, the LLVM project has always had llvm::Optional because the code base didn't support C++ 17 (which added std::optional). llvm::Optional had differently named member functions that std::optional, e.g., hasValue/has_value, getValue/value, and getValueOr/value_or. Recently LLVM moved to compiling with C++17. It was then viewed as bad to deviate from standard C++ conventions for names of member functions that new users were used to seeing. A C++ user expects to see has_value and not hasValue. New methods were added, the old were deprecated, and everybody migrated (at who knows what cost inside Google and other companies!). See: https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716.

What is the incentive for removing WireInit? Many existing projects use WireInit extensively.

I think these types of questions should be evaluated in terms of the language: Does having one way of initializing a wire make Chisel better? I'm not saying backwards compatibility is bad to aim for, but we need a cost metric other than "anything that requires an existing project to change anything is bad". Other, much larger and much more widely used projects (e.g. LLVM) go through this kind of healthy activity and continuous improvement/refinement at a surprising pace.

[aswaterman]

Given that we accept the premise that it is justifiable to deprecate things, why are we choosing to deprecate the version that people have spoken up in favor of using (and is easier to type)?

[sequencer]

How does this relate to VecInit?

I don't think this API relate to VecInit, the reason I'm proposing this deprecation is there are two API works exactly same, while it's tough to explain these to new users: "which one should be used, and why two APIs". VecInit doesn't have this issue.

What is the incentive for removing WireInit? Many existing projects use WireInit extensively.

The incentive is keeping two same APIs is misleading to new users, since WireDefault is merged later, and have a clear semantic, so I'm proposing deprecating the WireInit API.

Given that we accept the premise that it is justifiable to deprecate things, why are we choosing to deprecate the version that people have spoken up in favor of using (and is easier to type)?

Agree, but after #986 was in, I switch to WireDefualt, I think for both these two APIs, they are good to have a semantic: "define a hardware Wire, and set the default value with some constant value", however, to keep the codebase tidy, I'm proposing keeping Chisel it self as small as possible. Deprecating any one of these two API helps making things clean, like clean the compatibility layer did.

[aswaterman]

A SiFive-internal discussion considered the merits of Init vs. Default in this context. There was sympathy for the notion that WireInit and RegInit are not actually symmetric. But there was also some agreement that VecInit is consistent with WireInit, and it would be even more inconsistent to deprecate WireInit without also creating VecDefault and deprecating VecInit.

My opinion is that we should make no changes until we've thought through how to name all of these constructs, weighing Init vs. Default (vs. Reset vs ...) for each API. Then, we can do the new-API creation and old-API deprecation in bulk. I suspect we'll cause less churn that way.

(Other maintainers should feel free to dismiss this review if there's sufficient agreement to the contrary.)

[jerryz123]

VecInit doesn't have this issue.

Given that VecInit internally constructs a Wire with a default value, should VecInit also be renamed to VecDefault? The line of reasoning here opens the door to such questions. There is a a lot of code which uses VecInit and WireInit analogously, so VecInit/WireDefault could also be confusing to new users.

like cleaning the compatibility layer did.

The WireInit alias seems to be a much smaller maintenance burden than the Chisel2 compatibility layer

Analogously, the LLVM project has always had llvm::Optional

Fair point. Although in that case, LLVM changed to conform to existing patterns in the STL and C++ community. I don't think the benefit is quite as clear-cut here.

we need a cost metric other than "anything that requires an existing project to change anything is bad"

I agree, but that does not imply the cost metric shouldn't consider backwards compatibility as a factor at all. It ought to be considered that there is a growing collection of chisel generators without active maintainers, or maintainers who are interested in keeping up with the latest chisel. Preserving backwards compatibility such that these packages can be used in projects with newer chisel versions is good for the chisel community in general.

[schoeberl]

It is hard to fix namings after a product is in use. I still strongly support the "default" name, as we say when describing combinational circuits: "we assign a default value to a combinational circuit, especially to avoid describing latches" (not in Chisel but in V). We "initialize" a register on reset.

Yes, VecInit then does not fit as well. But then it becomes a slippery slope, as we can have a RegInit(VecInit(...)), which is now a reset value for a register.

When we talk about naming things: I don't like bits in the DecoupledIO. It should be called data. And I don't like DecoupledIO, as it should be called ReadyValidIO. OK, now I stop ;-)

[sequencer]

My opinion is: bad names doesn't matter, WireDefault/WireInit are both OK name, users will get used to it, but two names+same function is creepy... Maybe at the first time we shouldn't merge WireDefault...

[seldridge]

It ought to be considered that there is a growing collection of chisel generators without active maintainers, or maintainers who are interested in keeping up with the latest chisel. Preserving backwards compatibility such that these packages can be used in projects with newer chisel versions is good for the chisel community in general.

This is much stronger than considering the cost of changes to code. This is a very strong premise of "I will be able to compile a long dead project and an active, live project together with the newest version of Chisel." It then follows that any backwards-incompatible change to Chisel, however trivial, will violate this premise.

I am arguing that this premise is wrong. A long dead project stuck on an old version of Chisel should be able to be used in a new project with a newer version of Chisel, but they should be separately compiled and linked at the FIRRTL/MLIR or Verilog levels. The version of Chisel used to generate FIRRTL/Verilog is then per-project as opposed to the lowest common denominator (or the newest version, but requiring forever backwards compatibility). Admittedly, Chisel has never provided a good answer to how to do this and, in consequence, projects have commonly chosen monolithic compilation and assemblage via submodules (e.g., Rocket Chip, Chipyard, etc.). Additionally, libraries have been built on top of Chisel that assume monolithic compilation.

Definition/Instance was an approach to help with this, ideas like seldridge/chisel-separable are other experiments in this space.

WDYT?

(Thanks for this comment, though! I've had a very hard time articulating why seemingly innocuous changes like this are easy for other projects and hard for Chisel. I think your comment sheds a lot of light on the problem.)