Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revisit interaction between initialization and reset logic in Verilog backend #1089

Closed
sampsyo opened this issue Jul 8, 2022 · 2 comments · Fixed by #1426
Closed

Revisit interaction between initialization and reset logic in Verilog backend #1089

sampsyo opened this issue Jul 8, 2022 · 2 comments · Fixed by #1426
Labels
C: Calyx Extension or change to the Calyx IL C: FPGA Changes for the FPGA backend Type: Bug Bug in the implementation

Comments

@sampsyo
Copy link
Contributor

sampsyo commented Jul 8, 2022

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 #1085 (comment)

@sampsyo sampsyo added C: Calyx Extension or change to the Calyx IL Type: Bug Bug in the implementation C: FPGA Changes for the FPGA backend labels Jul 8, 2022
@rachitnigam
Copy link
Contributor

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
Copy link
Contributor

See the original bug (#284) and the fix (#288)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Calyx Extension or change to the Calyx IL C: FPGA Changes for the FPGA backend Type: Bug Bug in the implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants