search

chipsalliance / firrtl

Flexible Intermediate Representation for RTL
https://www.chisel-lang.org/firrtl/
Apache License 2.0
731 stars 177 forks source link

Moving from firrtl 1.3.3 to 1.4.1 changes emulator harness signal assignment behavior #2195

Open torurstrom opened 3 years ago

torurstrom commented 3 years ago

Checklist

What is the current behavior?

We are working on the Patmos project, which is a multicore processor written in Chisel. We have upgraded Chisel from 3.3.3 to 3.4.1, but this results in the Verilator generated emulator to stop working. Signals assigned by the emulator harness are shifted in time, or not assigned at all, compared to before, e.g., the program counter of a core was assigned the program entry point too late, resulting in the core not jumping.

We have compared the generated firrtl and verilog code. The firrtl code only has minor variable naming differences. However, the generated verilog code is quite different. Is there any known difference between the firrtl versions that changes either the generated hardware or the emulator harness signal assignment?

It should be noted that synthesizing and running the hardware on an FPGA board works as expected. Also, we assign signals in the harness like so: c->Patmos__DOT__cores_0__DOT__fetch__DOT__pcNext = (entry >> 2) - 1;

What is the expected behavior?

No behavior change in emulator harness signal assignment.

Steps to Reproduce

make emulator in the Patmos project, first using commit 30a259fcad01b690040894cc29e5db272131b387, and then commit 6129f412628f690493eb7799fa31a5e1221bd9f5. Compare the firrtl and verilog output in the hardware/build folder before and after.

Your environment

ekiwi commented 3 years ago

It should be noted that synthesizing and running the hardware on an FPGA board works as expected. Also, we assign signals in the harness like so: c->Patmos__DOT__cores_0__DOT__fetch__DOT__pcNext = (entry >> 2) - 1;

Not sure if this is related, but I am curious: Is this updating an internal signal? Are you annotating some signals with something like public_flat_rw in order for Verilator to expose them?

torurstrom commented 3 years ago

Correct, we have a harnessConfig.vlt containing, among others, public_flat_rw -module "Fetch" -var "pcNext" which exposes the pcNext internal signal.

ekiwi commented 3 years ago

Is there a reason why you changed from writing to e.g. relBaseNext to relBaseReg in this commit? Looking at your Chisel, it seems like the Next version if each signal should be exactly one cycle ahead of the Reg version.

torurstrom commented 3 years ago

Correct, that is how it worked before switching Chisel versions. Afterwards, we had to update both the register AND the next signal, as neither was enough on its own.

The emulator works when we do this. However, it seems quite fragile, as the harness has to be updated depending on the Chisel/firrtl version.

ekiwi commented 3 years ago

OK. To reveal my current intuition: I would guess that there is a 80% chance that the problem is with your harness. It might be relying on some specific property of the simulation that is not actually guaranteed by Verilator to hold. The fact that your design seems to work on the FPGA still would support this intuition.

So I believe you need to debug or rethink your harness. One way to go about this might be to just expose all signals that you need as inputs/outputs on the toplevel of your design. Like you would add a externalData + a externalOverride signal for each of these registers. When the externalOverride signal is asserted, you update the register with the externalData. The you can wire these two signal to the toplevel using the chisel BoringUtils.

torurstrom commented 3 years ago

Thanks for the pointer. I will check if we can do this without interfering with the proper top level.

ekiwi commented 3 years ago

Thanks for the pointer. I will check if we can do this without interfering with the proper top level.

You would probably have to add a feature flag. Like isEmulator: Boolean and then only generate the overwrites when you need to.

jackkoenig commented 3 years ago

Just to add to the "it's likely the test harness intuition." We've had numerous issues where things worked for a long time and then seemingly irrelevant changes caused Verilator simulations to start failing. See https://github.com/chipsalliance/chisel3/pull/1820 and https://github.com/chipsalliance/rocket-chip/pull/2635 for examples. The latter may not look relevant, but the way that issue manifested was bizarre with seemingly random assertions in the design firing erroneously.