Don't use declaration-assigns for wires representing mem ports

[albert-magyar]

Type of improvement: bug-fix
API impact: none
Backend code-generation impact: Avoids using declaration-assignments for "intermediate" wires created to represent the various fields of memory ports. Since these wires are create when the memory is declared, they may appear before the definition of whatever happens to drive them.
Desired merge strategy: squash

Contributor Checklist

• Did you add Scaladoc to every public function/method?
• Did you update the FIRRTL spec to include every new feature/behavior?
• 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 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?

Reviewer Checklist (only modified by reviewer)

• Did you add the appropriate labels?
• Did you mark the proper milestone (1.2.x, 1.3.0, 1.4.0) ?
• Did you review?
• Did you check whether all relevant Contributor checkboxes have been checked?
• Did you mark as Please Merge?

[albert-magyar]

Here is a minimal example of a circuit that would cause this Verilog declaration-use issue before this PR. This PR fixes it. I will add this as some kind of test as part of this PR.

circuit wdata_driver_decl_after_mem:
  module wdata_driver_decl_after_mem:
    input clk: Clock
    input addr: UInt<10>
    input wdata: UInt<8>[2]

    mem m:
      data-type => UInt<16>
      depth => 1024
      writer => w
      read-latency => 1
      write-latency => 1

    m.w.clk <= clk
    m.w.addr <= addr
    m.w.en <= UInt(1)
    m.w.mask <= UInt(1)
    node wdata_catted = cat(wdata[1], wdata[0])
    node wdata_actual = wdata_catted
    m.w.data <= wdata_actual

[ekiwi]

This PR fixes it. I will add this as some kind of test as part of this PR.

I wonder if we could add some more strict verilator linting to the regression tests. I am surprised that this problem wasn't caught in CI because it seems to also affect rocket chip. Apparent verilator accepts use before declaration?

[ekiwi]

I cannot get Verilator to complain about this problem. Not with -Wall and not by setting the default_nettype to none.

[albert-magyar]

Default nettype only comes into play when an undeclared identifier appears on the lhs of an assign.

[albert-magyar]

The change that caused the use-before-declaration was basically just a silly attempt at improving style that I tacked on to the actual substantive changes to the emitter in #2111. Most consumers of Verilog don't require declaration-before-rhs-reference for non-default nets since they try to be "declarative," but it's definitely better to emit universally acceptable code from a compiler.

@ekiwi @jackkoenig, since there is basically no way to write a test for this that will run in CI, can we just merge this?

[ekiwi]

since there is basically no way to write a test for this that will run in CI, can we just merge this?

🚢

[ekiwi]

Most consumers of Verilog don't require declaration-before-rhs-reference for non-default nets since they try to be "declarative," but it's definitely better to emit universally acceptable code from a compiler.

Just FYI, the tool that was complaining about this is VCS.

[jackkoenig]

This should have been backported right? Needed for #2172?