Add drawbacks

Author: rameloni

README.md

@@ -2,19 +2,26 @@
 
 Demo backend of the Tywaves project: a type based waveform viewer for Chisel
 and [Tydi-Chisel](https://github.com/abs-tudelft/Tydi-Chisel) circuits.
+
 This repo contains functions that parse the information of a Chisel circuit and the debug info emitted by the firtool
-compiler, simulate a circuit using ChiselSim and combine all the information to the
-[surfer-tywaves-demo](https://gitlab.com/rameloni/surfer-tywaves-demo) frontend (an extension of the Surfer waveform
-viewer written in Rust to support Chisel constructs).
+compiler, simulate a circuit using ChiselSim and combine all the information and pass them to the
+[surfer-tywaves-demo](https://gitlab.com/rameloni/surfer-tywaves-demo) frontend for visualization (an extension of the
+Surfer waveform viewer written in Rust to support Chisel constructs).
 
 Since it is a demo project, it has been tested and developed for a restricted set of examples (it may not work with
 all circuits). In the [features](#features) section, you can find the already supported features.
+This demo served only as an example to show how the waveform viewer can potentially look like and **for initial feedback
+and suggestions from the community**.
+
+> If you are interested in using the tool and have any feedback on its implementation, please open an issue or contact
+> me.
 
 Starting from this demo project, a better implementation and integration in the Chisel/Firrtl infrastructure will be
-developed that aims to address the issues
-discovered [here](https://github.com/rameloni/Tydi-Chisel-testing-frameworks-analysis).
+developed with the aim of solving the issues
+addressed [here](https://github.com/rameloni/Tydi-Chisel-testing-frameworks-analysis).
 
-> Do not use Verilog / System Verilog reserved keywords in Chisel circuit (i.e. `wire`, `reg`).
+
+> Do not use Verilog / System Verilog reserved keywords in your Chisel circuit (i.e. `wire`, `reg`).
 > In that case, `firtool` will change the `var_name` (which should be `wire`) field of the emitted HGLDD to get a legal
 > name as explained in the comments
 > [here](https://github.com/llvm/circt/blob/37fbe5e5f5c7a07064d02cea8bf4e8454178fc0e/lib/Target/DebugInfo/EmitHGLDD.cpp#L163C1-L175C2).
@@ -67,7 +74,7 @@ libraryDependencies += "com.github.rameloni" %% "tywaves-demo-backend" % "0.1.0-
 
 The `TywavesBackend` provides 2 simulators with functionalities to simulate a circuit
 through [svsim](https://github.com/chipsalliance/chisel/tree/main/svsim), emit VCD
-traces and of course generate the symbol table for the waveform viewer itself automatically:
+traces and generate the symbol table for the surfer-tywaves waveform viewer itself automatically:
 
 - [ParametricSimulator](./src/main/scala/tywaves/simulator/ParametricSimulator.scala): provides some generic features
   such as VCD trace emission, name the trace file, pass additional arguments to firtool before simulation, save the
@@ -76,10 +83,10 @@ traces and of course generate the symbol table for the waveform viewer itself au
   order to generate the symbol table for Tywaves waveform viewer and provides an option to launch the waveform viewer
   after the simulation
 
-> While `TywavesSimulator` is central part of the Tywaves project and its functionalities are not fully supported
-> yet, `ParametricSimulator` is
-> should be able to simulate any Chisel circuit. In case you need to simulate a circuit that is not supported
-> by `TywavesSimulator`, you can use `ParametricSimulator`.
+> While `TywavesSimulator` is a central part of the Tywaves project and its functionalities are not fully supported
+> yet, the `ParametricSimulator` is able to simulate any Chisel circuit. In case you need to simulate a circuit that is
+> not supported by `TywavesSimulator`, you can use `ParametricSimulator` to emit a VCD trace (however, you will not have
+> a "chisel" representation of the signals in the waveform viewer).
 >
 > If you want to try the functionalities of `Tywaves` then `TywavesSimulator` is the right choice.
 > But, if you want to visualize waveforms of any chisel circuit without issues related to features not supported yet,
@@ -92,6 +99,7 @@ The following example shows how it is possible also to:
   workspace of svsim)
 - Launch the waveform viewer after the simulation
 - Use tywaves and expect API to test the circuit
+
 ### Use TywavesSimulator
 
 ```scala
@@ -119,6 +127,7 @@ class GCDTest extends AnyFunSpec with Matchers {
 ```
 
 ### Use ParametricSimulator
+
 ```scala
 import tywaves.simulator.ParametricSimulator._
 import tywaves.simulator.simulatorSettings._
@@ -145,7 +154,8 @@ class GCDTest extends AnyFunSpec with Matchers {
 
 # Example output
 
-The following images show the classic and tywaves waveform visualization of the [GCD](./src/test/scala/gcd/GCD.scala) module.
+The following images show the classic and tywaves waveform visualization of the [GCD](./src/test/scala/gcd/GCD.scala)
+module.
 It is possible to see that the left picture does not provide any information about Chisel level types and hierarchy.
 
 ```scala
@@ -185,6 +195,9 @@ class GCD extends Module {
     - [ ] Represent vectors
     - [ ] Represent enums
     - [ ] Represent hierarchical modules
+        - [x] Generic submodules (all different types of modules)
+        - [x] Variants of the same module (i.e. parametric module)
+        - [ ] Instances of the same module
     - [ ] For loops code generation
     - [ ] Reg with init
 
@@ -193,14 +206,26 @@ class GCD extends Module {
 The following diagram shows the main components of the demo project and how they interact with each other.
 ![Tywaves backend diagram](./images/tywaves-backend-diagram.png)
 
-It retrieves, parses and finally maps together the Intermediate Representation (IR) of the Chisel, Firrtl and debug info
-emitted by the firtool (HGLDD) to output a symbol table that can be used by the frontend to display the waveform.
-It aims to map each high level signal (Chisel) to the low level signal (System Verilog) and vice versa. Usually, HGLDD
-would be enough, but it does provide only information about FIRRTL-to-SystemVerilog mapping, so it does not contain user
-types information.
+This backend retrieves, parses and finally maps together the Intermediate Representations (IR) of the Chisel, Firrtl and
+debug info emitted by the firtool (HGLDD) to output a symbol table that can be used by the frontend to display the
+waveform.
+It aims to map each high level signal (Chisel) to the low level signal (in System Verilog and in the VCD/FST trace) and
+vice versa. In this way it would be possible to access a variable/signal value from any waveform viewer able to support
+a **multi-level typed** view. For this demo I managed to do so by:
+
+- parsing Chisel IR, Firrtl IR, and the HGLDD (debug info emitted by firtool to link verilog and firrtl)
+- retrieving and joining signals together by identifying IDs (shared between IRs) based on signal names and source
+  locators
+- emitting the symbol table suited for the waveform viewer
+
+However, this approach has some issues associated with the IRs parsing and mapping. The [drawbacks](#drawbacks) section
+explains them and suggests a solution to them that I will implement.
+
+Considering only Firrtl, using the HGLDD file would be enough, but it does provide only information about
+FIRRTL-to-SystemVerilog mapping, so it does not contain user types information.
 
 In this small example if I use only HGLDD I would be able to see that they are both bundles, but it is not possible to
-see that they are actually `MyFloat` and `IntCoordination` respectively. Also `Bool`, `UInt`, `SInt` would not be
+see that they are actually `MyFloat` and `IntCoordinates` respectively. Also `Bool`, `UInt`, `SInt` would not be
 retrieved from HGLDD/FIRRTL only. From here the reason to use Chisel IR to get the user types information.
 
 ```scala
@@ -210,18 +235,48 @@ class MyFloat extends Bundle {
   val significand = UInt(23.W)
 }
 
-class IntCoordination extends Bundle {
+class IntCoordinates extends Bundle {
   val x = SInt(32.W)
   val y = SInt(32.W)
 }
 ```
 
 ## Drawbacks
 
-Chisel IR is an **internal** IR, and it is not meant to be used by external tools. It is not stable, and it can change
-basing on additional future features. It will be really hard to maintain this project for future Chisel versions.
+The current approach accesses and uses the Chisel IR which is private to the package `chisel3` since it is part of the
+chisel **internals,** and it is not meant to be used by external tools. It is not stable, it may change and this may
+compromise compatibility with future chisel versions. Using Chisel IR in this way will make relatively hard to maintain
+this project for future Chisel versions.
+
+Mapping different IRs together requires to find common characteristics between them. Different IRs have different
+information, reserved keywords and syntax. This can lead to different ways to represent variables and changes to their
+names. For example, a variable `x` child of a bundle `b` may be represented with `b_x` in Verilog, but it is not
+guaranteed when there are some conflicting variables.
+Therefore, this methodology requires to find an ID for each signal which is unique within the same IR, but it
+is shared between IRs. Finding an ID with these characteristics is not trivial at all since it really depends on the
+characteristics emitted during the different elaboration/compilation phases. These information, even if available,
+should remain consistent between different versions of the tools but this may not be guaranteed.
+Currently, the IDs (`ElId`) used by the tool to join the different IRs are based on source locators (where a variable is
+declared in a **source** module, not instance) contained in the IRs and names of the variables. However, this may cause
+issues when signals and modules are generated using for loops. This explains why multiple instances of the same module
+are not supported yet (the source locators of internal signals of multiple instances of the same module are the same).
+Furthermore, finding the original chisel name from HGLDD requires manipulation based on the transform function used.
+
+```scala
+case class ElId(source: String, row: Int, col: Int, name: String)
+```
+
+Finally, this tool relies on HGLDD which is a file realized for Synopsys tools and its format is not stable since it
+mainly depends on what Synopsys will need in the future.
+
+These issues reveal the need for a more stable and consistent way to map different IRs together. Parsing the IRs
+externally and joining them basing on a "guessed and unstable" ID is not an optimal solution (guessed and unstable since
+it depends on internal characteristics of compilers).
 Therefore, I planned to "integrate" a functionality to directly transfer Chisel information to FIRRTL. In this way,
 the `firtool` would be able to access all the needed information for `surfer-tywaves-demo` to render the signals.
 This would also allow to simplify the process that `tywaves-demo-backend` currently does to generate the symbol table,
 improving performances. And it may extend the support to other languages/dialects in
-the [CIRCT](https://circt.llvm.org/) ecosystem.
+the [CIRCT](https://circt.llvm.org/) ecosystem.
+
+Despite the drawbacks, this demo successfully shows the potential of the Tywaves project and the feasibility of a Typed
+Waveform Viewer for Chisel circuits.

src/test/scala/hierarchicalmodules/MultiBlink.scala

@@ -3,14 +3,14 @@ package hierarchicalmodules
 import chisel3._
 
 class SubBlink(n: Int) extends Module {
-  val iox = IO(new Bundle {
-    val enablex = Input(Bool())
-    val ledx    = Output(Bool())
+  val io = IO(new Bundle {
+    val enable = Input(Bool())
+    val led    = Output(Bool())
   })
   val blinkerx = Module(new Blink(n))
 
-  blinkerx.io.enable := iox.enablex
-  iox.ledx           := blinkerx.io.led
+  blinkerx.io.enable := io.enable
+  io.led             := blinkerx.io.led
 }
 
 class AnotherModule extends Module {
@@ -30,28 +30,19 @@ class AMultiBlink extends Module {
 
   val blinker:  SubBlink = Module(new SubBlink(5))
   val blinker2: Blink    = Module(new Blink(3))
+  val blinker3: Blink    = Module(new Blink(2))
 
-  blinker.iox.enablex := io.enable
-  blinker2.io.enable  := io.enable
+  blinker.io.enable  := io.enable
+  blinker2.io.enable := io.enable
+  blinker3.io.enable := io.enable
 
   when(anotherModule.out === 1.U) {
-    blinker.iox.enablex := false.B
-    blinker2.io.enable  := false.B
+    blinker.io.enable  := false.B
+    blinker2.io.enable := false.B
   }
   io.leds :=
-    (blinker.iox.ledx << 3).asUInt + (blinker.iox.ledx << 2).asUInt + (blinker.iox.ledx << 1).asUInt + (blinker.iox.ledx << 0).asUInt
-//  val blinks = Seq.fill(4)(Module(new Blink(5)))
-
-//  for (i <- 1 until 5) {
-//    val b = Module(new Blink(i))
-//    b.io.enable := io.enable
-//    io.leds(i - 1) := b.io.led
-//  }
+    (blinker.io.led << 3).asUInt + (blinker.io.led << 2).asUInt + (blinker.io.led << 1).asUInt + (blinker.io.led << 0).asUInt
 
-//  blinks.zip(io.leds).foreach { case (b, l) =>
-//    b.io.enable := io.enable
-//    l           := b.io.led
-//  }
 }
 
 object MainMultiBlink extends App {