calyx icon indicating copy to clipboard operation
calyx copied to clipboard

Revisit interaction between initialization and reset logic in Verilog backend

Open sampsyo opened this issue 2 years ago • 2 comments

We disabled initialization when targeting the Xilinx toolchain in #1085, but this is a hack. We should figure out what we're doing wrong here and fix it once and for all, so that the same Verilog works in both Xilinx and Verilator settings. Then we should remove the --disable-init flag from the relevant fud stage.

Copying some text from the aforementioned thread, although there's other good stuff in there too:


Seems good if it works! Just thinking through the implications here for a sec… this pass disables all our "automatic initialization" stuff when generating Verilog but only for Xilinx execution. I hope that none of the code is relying on any kind of initialization… @andrewb1999's original advice in #1071 suggests that it was not necessary to get things working:

It's a bad idea to initialize logic (especially resets) that are used as wires rather than registers. I just commented out the entire initial block in the verilog which prevented issues with std_regs not being reset properly (currently none of the logic defined in the main module is being used as a register so this is fine to do).

But I wonder if this doesn't actually indicate a deeper problem. Namely, perhaps the initialization we're doing is wrong not just in a Xilinx context but for all Verilog generation—and we just need to back off on some of that altogether. It would take a little more digging into how we currently initialize things to know for sure, but perhaps there's a subtler change to be made here.

I would still recommend (1) forging ahead with this PR as-is, if it actually seems to improve things for Xilinx execution, and then (2) revisiting this broader "is our initialization stuff correct?" question later, with a goal toward removing the special --disable-init treatment in this stage.

Originally posted by @sampsyo in https://github.com/cucapra/calyx/issues/1085#issuecomment-1177868543

sampsyo avatar Jul 08 '22 20:07 sampsyo

For reference: We originally implemented the initial block stuff because we kept encountering a zero-time reset bug in Verilator. We can try removing the initial blocks entirely and see if we run into problems. Unfortunately its one of those things that keeps biting us in the back over and over.

rachitnigam avatar Jul 12 '22 17:07 rachitnigam

See the original bug (https://github.com/cucapra/calyx/issues/284) and the fix (https://github.com/cucapra/calyx/pull/288)

rachitnigam avatar Jul 12 '22 17:07 rachitnigam