Can't get simulation VCD dumps for View fields

[tilk]

Using Views instead of Records has an unfortunate consequence for debugging. In Record, each of the fields had a separate Signal, which made them available for VCD dumping. In contrast, Views doesn't introduce new signals, but allows to access fragments of an existing value (including a Signal). In practice, this means that there is typically one Signal for the entire View. Unfortunately, VCD dumps display only whole Signals, and the traces argument for write_vcd handles only Signals, too. In consequence, there is no way to see View fields in VCD dumps.

[whitequark]

In practice, this means that there is typically one Signal for the entire View.

This is an expected and intended consequence of the design. (There is no way to implement View, in general, with multiple signals due owing to the existence of union views.)

Unfortunately, VCD dumps display only whole Signals, and the traces argument for write_vcd handles only Signals, too. In consequence, there is no way to see View fields in VCD dumps.

In the short term the only way to fix this is to add a decoder for all signals created from views. Would that be sufficient for you?

[tilk]

This is an expected and intended consequence of the design. (There is no way to implement View, in general, with multiple signals due owing to the existence of union views.)

This is clear to me, I guess I didn't state it clearly enough. I don't think that's wrong; just that it, in the current implementation of VCD dumps, this interferes with getting useful traces.

In the short term the only way to fix this is to add a decoder for all signals created from views. Would that be sufficient for you?

This would be useful in general, but not sufficient for us. The availability of the per-field dumps was often extremely helpful for debugging complex components, and we rely on this functionality for development.

[whitequark]

I'll see how I can fit this into our development timeline and get back to you.

For now (i.e. to make sure you do not lose this functionality as Record gets removed) my suggestion is to design a wrapper that introspects the View and adds assign statements to the module corresponding to every field.

[whitequark]

Effectively this issue is asking for a decoupling of the Signals in the source code and the VCD lines, which isn't something that we can easily, or without a design cycle, add right now.

[tilk]

Effectively this issue is asking for a decoupling of the Signals in the source code and the VCD lines, which isn't something that we can easily, or without a design cycle, add right now.

After inspecting the implementation of the VCD dump code, I figured as much. So, unfortunately, it looks like we need to work around this issue for now.

[whitequark]

It's actually worse than just the Python VCD dump code--we need to figure out how to handle this for CXXRTL as well.

[whitequark]

So I'm considering adding a hack specifically to enable migration to views to the Python simulator and dealing with it properly later, but I'm not sure yet. How invasive would a workaround be?

[tilk]

How invasive would a workaround be?

Still needs to be explored. We collect signals used in debugging after all Elaboratables are constructed, but before they are passed to the simulator. This probably means that systematically generating the required Signals and assigning them to the right Modules would require some hackery to do transparently to the API users.

[whitequark]

I think I'll prioritize this for 0.4 so you don't have to spend effort on a workaround.

[bieganski]

the issue is real for me as well. I attach the workaround that I use, in hope that someone will find it useful.

    def to_record(v: data.View) -> rec.Record:
        members : dict = data.Layout.cast(v._View__orig_layout).members
        return rec.Record(rec.Layout([x for x in members.items()]))

    def record_view_connect_statements(r: rec.Record, v: data.View) -> list:
        members : dict = data.Layout.cast(v._View__orig_layout).members
        return [getattr(r, name).eq(getattr(v, name)) for name in members]

and in Elaboratable:

      def __init__(...):
          self.view = Signal(layout)
          self.record = to_record(self.view)
      
      def elaborate(...):
           records_views = [
               (self.view, self.cur_COMMAND),
               ...
           ]

           for r, v in records_views:
               m.d.comb += record_view_connect_statements(r, v)

In order to collect fields traces to .gtkw file, I do:

vcd_traces = [
        *dut.record.fields.values(),

not sure about when _View__orig_layout no longer is valid, but for my simple use case it works fine.

[whitequark]

@bieganski @tilk There is a PR available now that adds support for fields in VCD dumps, which is aimed at inclusion in the 0.4 release: #967. Could you test it please?

[tilk]

Thanks for the info, we will test it soon.

[whitequark]

Now that #967 was merged, the parts of this feature that go into version 0.4 are done. It still needs an RFC for version 0.5.

[tilk]

@whitequark This is almost what we need. Almost, because we rely on the traces feature of write_vcd to automatically generate a nice .gtkw file. This allows us to quickly view the relevant signals without looking for them; and for debugging some issues, we need to view a lot of signals. Unfortunately, View fields are rejected in extra_signals with an exception like:

TypeError: Object (slice (sig data_in) 0:16) is not an Amaranth signal

[whitequark]

Unfortunately, View fields are rejected in extra_signals with an exception like:

Oh, I see: we used to have traces corresponding only to signals, but now we also have traces corresponding to views. We'll take a look.

[whitequark]

@tilk Would this patch suffice?

diff --git a/amaranth/sim/pysim.py b/amaranth/sim/pysim.py
index f6d3e5ca..bc5b1ec1 100644
--- a/amaranth/sim/pysim.py
+++ b/amaranth/sim/pysim.py
@@ -64,9 +64,10 @@ class _VCDWriter:
 
         trace_names = SignalDict()
         for trace in traces:
-            if trace not in signal_names:
-                trace_names[trace] = {("bench", trace.name)}
-            self.traces.append(trace)
+            for trace_signal in trace._rhs_signals():
+                if trace_signal not in signal_names:
+                    trace_names[trace_signal] = {("bench", trace_signal.name)}
+                self.traces.append(trace_signal)
 
         if self.vcd_writer is None:
             return

[tilk]

@whitequark This looks good enough, thank you.

[whitequark]

@tilk #973

[whitequark]

@tilk That's now merged--I would be very happy if you were able to test Amaranth main branch in preparation for the 0.4 release and report any issues that arise. In particular please let us know if #957 causes any trouble for you.

[tilk]

@whitequark Sure, I'll report back when it's done. I'm optimistic with regard to #957 - we sure welcome more type strictness.

[whitequark]

we sure welcome more type strictness.

This is the direction in which I will continue to push the language, so any feedback regarding run-time type strictness is warmly welcome--feel free to file an issue whenever you think a type confusion bug could be addressed by stricter type checking.

I also do plan to introduce Pyright/Pylance-compatible type annotations (specifically--it seems to be considerably more advanced than mypy); that's a longer-term project but watching your project made me realize that it is actually reasonably practical and useful to do so, something I have not previously believed.

[tilk]

We found a weird issue with VCD writing. Sometimes having signals with identical names leads to an error:

from amaranth import *
from amaranth.sim import *

class Lol(Elaboratable):
    def __init__(self):
        self.x = Signal()
        self.y = Signal(name="y")
        self.y2 = Signal(name="y")

    def elaborate(self, platform):
        m = Module()

        m.d.sync += self.x.eq(1)

        return m

mod = Lol()

sim = Simulator(mod)
sim.add_clock(1e-6)

with sim.write_vcd("dump.vcd", "dump.gtkw", traces=[mod.y, mod.y2]):
    sim.run_until(1e-3)

Traceback (most recent call last):
  File "/home/tilk/kuznia-rdzeni/coreblocks/dupa2.py", line 22, in <module>
    with sim.write_vcd("dump.vcd", "dump.gtkw", traces=[mod.y, mod.y2]):
  File "/usr/lib/python3.11/contextlib.py", line 137, in __enter__
    return next(self.gen)
           ^^^^^^^^^^^^^^
  File "/home/tilk/kuznia-rdzeni/coreblocks/.venv/lib/python3.11/site-packages/amaranth/sim/pysim.py", line 324, in write_vcd
    vcd_writer = _VCDWriter(self._fragment,
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/tilk/kuznia-rdzeni/coreblocks/.venv/lib/python3.11/site-packages/amaranth/sim/pysim.py", line 103, in __init__
    vcd_var = self.vcd_writer.register_var(
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/tilk/kuznia-rdzeni/coreblocks/.venv/lib/python3.11/site-packages/vcd/writer.py", line 166, in register_var
    raise KeyError(
KeyError: 'Duplicate var y in scope bench'

[whitequark]

Ah, this is because traces are not dedupicated. Can you file this as a separate bug please?

[whitequark]

RFC written: amaranth-lang/rfcs#65

[whitequark]

Closing in favor of #1293.