Semantic Inconsistency in Intermediate FIRRTL File During Compilation: Incorrect reg Emission Instead of node and Loss of Memory readUnderWrite Behavior

[linmoIO]

Type of issue: Bug Report

Please provide the steps to reproduce the problem: Chisel Version: v3.5.6
FIRRTL Version: v1.5.6
Treadle Version: v1.5.6

We ran the official tests for each of the following cases (without any modifications), and then examined the .fir files in the test_run_dir. We observed inconsistencies between the .fir files and the source code. The details are provided below.

1. ShiftResetTester

   • Source Code: src/test/scala/chiselTests/Reg.scala, Line 54
   • Description: When n = 0, the register sr should have a constant value, which should be compiled to a node. (This is observable when emitting FIRRTL, where the translation results in a node.) However, when emitting Verilog (which generates a .fir file in the same directory), the register sr is compiled to a reg, leading to an inconsistency with the source code. Initially, the value in reg sr is either random or zero (depending on the settings), rather than the expected constant value.

2. ShiftTester

   • Source Code: src/test/scala/chiselTests/Reg.scala, Line 44
   • Description: This issue is similar to that in ShiftResetTester.

3. SyncReadMemWriteCollisionTester

   • Source Code: src/test/scala/chiselTests/Mem.scala, Line 37
   • Description: When emitting to Verilog, the memory readUnderWrite behavior is lost in the .fir file generated in the same directory.

What is the current behavior? There are cases of semantic inconsistencies between the .fir intermediate files generated by Chisel's emit verilog and the source code (including incorrect compilation and loss of semantic information).

What is the expected behavior? The semantic behavior of the .fir intermediate files generated by Chisel's emit verilog is consistent with the source code.

Please tell us about your environment:

• version: v3.5.6
• OS: Ubuntu 24.04 LTS

Other Information

Use emit verilog to generate the .fir in the same directory (test_run_dir) instead of directly using emit firrtl. I observed that in the FIRRTL repository source code, in src/main/scala/firrtl/ir/Serializer.scala at Line 303, the readUnderWrite field is lost during the emission.

What is the use case for changing the behavior? I have mentioned it before: the official test code for ShiftResetTester, ShiftTester, and SyncReadMemWriteCollisionTester in the Chisel v3.5.6 repository.

[seldridge]

For (1) and (2), are you sure that the FIRRTL being produced is incorrect? Those tests are written as forAll which I think is sweeping over different parameters and possibly overwriting the same .fir file multiple times. That may be leading to some confusion on what is going on.

When I created a single-file version of this, I don't see that:

//> using scala "2.13.12"
//> using repository sonatype-s01:snapshots
//> using dep "org.chipsalliance::chisel::7.0.0-M2+160-d564445d-SNAPSHOT"
//> using plugin "org.chipsalliance:::chisel-plugin::7.0.0-M2+160-d564445d-SNAPSHOT"
//> using options "-unchecked", "-deprecation", "-language:reflectiveCalls", "-feature", "-Xcheckinit", "-Xfatal-warnings", "-Ywarn-dead-code", "-Ywarn-unused", "-Ymacro-annotations"

import chisel3._
import chisel3.util.{Counter, ShiftRegister}
import circt.stage.ChiselStage

class Foo(n: Int) extends Module {
  val (cntVal, done) = Counter(true.B, n)
  val start = 23.U
  val sr = ShiftRegister(cntVal + start, n)
  when(done) {
    assert(sr === start)
    stop()
  }
}

object Main extends App {
  println(
    ChiselStage.emitCHIRRTL(
      new Foo(0)
    )
  )

  println(
    ChiselStage.emitSystemVerilog(
      new Foo(0),
      firtoolOpts = Array(
        "-strip-debug-info",
        "-disable-all-randomization",
        "-enable-layers=Verification,Verification.Assert,Verification.Assume,Verification.Cover"
      )
    )
  )
}

Running the above with scala-cli 4526.scala, I get the following FIRRTL:

FIRRTL version 4.0.0
circuit Foo :
  layer Verification, bind, "Verification" :
    layer Assert, bind, "Verification/Assert" :
    layer Assume, bind, "Verification/Assume" :
    layer Cover, bind, "Verification/Cover" :
  public module Foo :
    input clock : Clock
    input reset : UInt<1>

    wire cntVal : UInt
    connect cntVal, UInt<1>(0h0)
    wire done : UInt<1>
    connect done, UInt<1>(0h0)
    when UInt<1>(0h1) :
      connect done, UInt<1>(0h1)
    node _sr_T = add(cntVal, UInt<5>(0h17))
    node sr = tail(_sr_T, 1)
    when done :
      node _T = eq(sr, UInt<5>(0h17))
      node _T_1 = eq(reset, UInt<1>(0h0))
      layerblock Verification :
        layerblock Assert :
          intrinsic(circt_chisel_ifelsefatal<format = "Assertion failed at 4526.scala:16\n", label = "chisel3_builtin">, clock, _T, _T_1)

      layerblock Verification :
        node _T_2 = eq(reset, UInt<1>(0h0))
        when _T_2 :
          stop(clock, UInt<1>(0h1), 0) : stop

// Generated by CIRCT firtool-1.89.0

// Users can define 'STOP_COND' to add an extra gate to stop conditions.
`ifndef STOP_COND_
  `ifdef STOP_COND
    `define STOP_COND_ (`STOP_COND)
  `else  // STOP_COND
    `define STOP_COND_ 1
  `endif // STOP_COND
`endif // not def STOP_COND_
module Foo(
  input clock,
        reset
);

  `ifndef SYNTHESIS
    always
      if ((`STOP_COND_) & ~reset)
        $finish;
    end // always
  `endif // not def SYNTHESIS
endmodule

That has sr as a node.

For (3), this was a long-standing bug in that Chisel never emitted these attributes. That was fixed in https://github.com/chipsalliance/firrtl/commit/d9da6b17189162751448fcb2240b25cadf31c866. However, it looks like that never made a version of the Scala-based FIRRTL compiler (SFC) as I only see it on the 1.5.x branch (and not on a tag).

Newer Chisel versions will emit this. However, there is a separate bug in CIRCT (the new FIRRTL compiler) where these attributes are ignored: https://github.com/llvm/circt/issues/787 It was originally considered to just drop all behaviors other than undefined, however, this was rejected here: https://github.com/chipsalliance/chisel/pull/2948 On Chisel main, those tests are disabled for this reason.

Very high level, though, Chisel 3.5.6 is quite old at this point and the SFC is no longer receiving updates. (3) can get fixed on llvm/circt if there is someone motivated to open a PR. However, I don't expect this to be ported to the SFC as that project is archived.

[linmoIO]

Thank you very much for your response. Regarding (3), I no longer have any doubts, but I am still confused about (1) and (2). In fact, I observed the bugs related to (1) and (2) on Chisel v3.5.6, not the latest CIRCT-based version (which seems to be the one you compiled).

Regarding the issue of overwriting .fir files, in the test_run_dir, there are subfolders stored with timestamps, and the contents within them are independent (as shown in the image below).

However, this is not important. I believe the inconsistency I mentioned earlier may not have been observed by you because you are not using Chisel v3.5.6. Below is the intermediate .fir file I obtained during the emit verilog operation:

FIRRTL version 1.1.0
circuit ShiftResetTester :
  module ShiftResetTester :
    input clock : Clock
    input reset : UInt<1>
    output io : { }

    wire cntVal : UInt @[Counter.scala 61:73]
    cntVal <= UInt<1>("h0") @[Counter.scala 61:73]
    wire done : UInt<1> @[Counter.scala 117:24]
    done <= UInt<1>("h0") @[Counter.scala 117:24]
    when UInt<1>("h1") : @[Counter.scala 118:16]
      done <= UInt<1>("h1") @[Counter.scala 118:23]
    node _sr_T = add(cntVal, UInt<5>("h17")) @[Reg.scala 57:33]
    node _sr_T_1 = tail(_sr_T, 1) @[Reg.scala 57:33]
    reg sr_r : UInt, clock with :
      reset => (reset, UInt<1>("h1")) @[Reg.scala 35:20]
    when UInt<1>("h1") : @[Reg.scala 36:18]
      sr_r <= _sr_T_1 @[Reg.scala 36:22]
    reg sr : UInt, clock with :
      reset => (reset, UInt<1>("h1")) @[Reg.scala 35:20]
    when UInt<1>("h1") : @[Reg.scala 36:18]
      sr <= sr_r @[Reg.scala 36:22]
    when done : @[Reg.scala 58:14]
      node _T = eq(sr, UInt<1>("h1")) @[Reg.scala 59:15]
      node _T_1 = bits(reset, 0, 0) @[Reg.scala 59:11]
      node _T_2 = eq(_T_1, UInt<1>("h0")) @[Reg.scala 59:11]
      when _T_2 : @[Reg.scala 59:11]
        node _T_3 = eq(_T, UInt<1>("h0")) @[Reg.scala 59:11]
        when _T_3 : @[Reg.scala 59:11]
          printf(clock, UInt<1>("h1"), "Assertion failed\n    at Reg.scala:59 assert(sr === (if (n == 0) cntVal + 23.U else 1.U))\n") : printf @[Reg.scala 59:11]
        assert(clock, _T, UInt<1>("h1"), "") : assert @[Reg.scala 59:11]
      node _T_4 = bits(reset, 0, 0) @[Reg.scala 60:9]
      node _T_5 = eq(_T_4, UInt<1>("h0")) @[Reg.scala 60:9]
      when _T_5 : @[Reg.scala 60:9]
        stop(clock, UInt<1>("h1"), 0) : stop @[Reg.scala 60:9]

As can be seen, sr here is emitted as reg rather than node. I am curious about the reason behind this behavior.