google / skywater-pdk

Open source process design kit for usage with SkyWater Technology Foundry's 130nm node.
https://skywater-pdk.rtfd.io
Apache License 2.0
2.98k stars 390 forks source link

Check the verilog for good style and run automated style / lint / formatting checks on it #85

Open mithro opened 4 years ago

mithro commented 4 years ago

The verilog in this repository has been formatted to follow a consistent style but that style has not been documented anywhere. We should document the style and add automated checks which make sure the style stays consistent.

We may want to move to the Verilog style of be that used by LowRISC projects. That way we could use the Verible tool for checking compliance and eventually auto-formatting.

We should also check that the automatic generated verilog output is consistent with the approved style too.

mithro commented 4 years ago

FYI - @hzeller

mithro commented 4 years ago

I believe this is blocked by issues like;

hzeller commented 4 years ago

also specify...endspecify

Currently, verible formats about 22k out of the 31k verilog files.

I'll have to sieve through the other error messages; probably not too terrible - here are the unique ones:

     72 syntax error, rejected "$hold" (https://github.com/google/verible).
    297 syntax error, rejected "$recrem" (https://github.com/google/verible).
   1662 syntax error, rejected "$setuphold" (https://github.com/google/verible).
   1221 syntax error, rejected "$width" (https://github.com/google/verible).
   5838 syntax error, rejected "cell" (https://github.com/google/verible).
   2031 syntax error, rejected "endspecify" (https://github.com/google/verible).
   2256 syntax error, rejected "(" (https://github.com/google/verible).
  11463 syntax error, rejected "if" (https://github.com/google/verible).
mithro commented 4 years ago

Could you group the errors by verilog file type? IE .blackbox.v, .tb.v, .symbol.v?

I'm guessing all the specify related errors come from the XXX.specify.v files?

hzeller commented 4 years ago

Most of them actually happen in specify.v files, only the cell syntax error happens in *.v

     72 .specify.v  syntax error, rejected "$hold" (https://github.com/google/verible).
    297 .specify.v  syntax error, rejected "$recrem" (https://github.com/google/verible).
   1221 .specify.v  syntax error, rejected "$width" (https://github.com/google/verible).
   1662 .specify.v  syntax error, rejected "$setuphold" (https://github.com/google/verible).
   2031 .specify.v  syntax error, rejected "endspecify" (https://github.com/google/verible).
   2256 .specify.v  syntax error, rejected "(" (https://github.com/google/verible).
   5838 .v  syntax error, rejected "cell" (https://github.com/google/verible).
  11463 .specify.v  syntax error, rejected "if" (https://github.com/google/verible).
mithro commented 4 years ago

Great!

So it sounds like if we fix the "rejected 'cell'" issue we can "auto-format" everything apart from the XXX.specify.v files?

hzeller commented 4 years ago

Yep, looks like.

mithro commented 4 years ago

Actually I think the 'cell' issue is caused by it being invalid Verilog -- see #152 ?

fangism commented 4 years ago

It is a 2001 keyword:

/* The 1364-2001 configuration tokens. */
%token TK_cell "cell"