chipsalliance / rocket-chip

Rocket Chip Generator
Other
3.16k stars 1.11k forks source link

Chisel generates invalid nesting of always blocks and/or initializations. #3677

Open leviathanch opened 3 weeks ago

leviathanch commented 3 weeks ago

When running yosys -p "read_verilog -sv generated-src/freechips.rocketchip.system.LitexConfig_small_1_1.sv" on my newly generated System Verilog code, after finally extending Yosys for the new syntax it didn't know yet it correctly points out that this code Chisel generates isn't valid.

always @(posedge clock) begin automatic logic _T_2; // src/main/scala/chisel3/util/Decoupled.scala:52:35 automatic logic _T_5; // src/main/scala/chisel3/util/Decoupled.scala:52:35

Is there a flag for telling Chisel to not do initializations within always blocks or do I have to hack Yosys so that it moves initialization out of such always blocks?

In other words. Who's going to fix this? Rocket Chip/Chisel or Yosys?

jackkoenig commented 3 weeks ago

firtool takes lowering options, one of which fixes this, try --lowering-options=disallowLocalVariables (comma separated if you're using multiple lowering options which you probably should be). Per what lowering options should you be using, I think Chipyard has some (and has a flow for using Yosys: https://chipyard.readthedocs.io/en/latest/VLSI/Sky130-OpenROAD-Tutorial.html), perhaps worth looking into if you haven't already.

That emission seems worth a bug report to CIRCT, I think most firtool users use that lowering option so it's likely we just haven't noticed this issue yet.

seldridge commented 3 weeks ago

The underlying problem is that Yosys doesn't support all of what is legal SystemVerilog. That emission is legal and Yosys doesn't support it. (Though given the size of the SystemVerilog spec, there is likely always some corner that you could find that some tool doesn't support.) The solution to this, as Jack has said, is with CIRCT lowering options. The advisable ones are listed on this page (direct link to the Yosys secction): https://circt.llvm.org/docs/VerilogGeneration/#yosys I do think Yosys eventually added packed array support, so that additional option advised there may no longer be necessary.

More context: it was extremely annoying that Yosys didn't support this part of the spec. Without automatic logic, SystemVerilog has no true notion of a lexically scoped temporary. From a compiler perspective, if you have some common expression that you want to spill inside in always block if you can't use automatic logic, then you have to spill to a net at the top level of the module. 🥲 That lowering option will do this spilling to top-level wires.