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

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

Open
leviathanch opened this issue Aug 21, 2024 · 2 comments

Comments

@leviathanch
Copy link
Contributor

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

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

seldridge commented Aug 21, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants