SymbiFlow / XilinxUnisimLibrary

Apache 2.0 licensed copy of the Xilinx Unisim library.
Apache License 2.0
9 stars 1 forks source link

Make the files consistently formatted #1

Open mithro opened 4 years ago

mithro commented 4 years ago

Maybe we can use google/verible for this?

hzeller commented 4 years ago

This is how the formatting would look like with current verible; some look better, some worse. https://github.com/hzeller/XilinxUnisimLibrary/commit/1a30641df1689b80324f8888d445d08f00281d16

A lot of files could not be processed, possibly due to some large initializer lists (a known issue with verible) https://github.com/hzeller/XilinxUnisimLibrary/commit/a1758ef9a671a0fb979eb9f69ae426ec89e6d5ac

Also one crash on ./verilog/src/unisims/PCIE4CE4.v

CC: @fangism

fangism commented 4 years ago

Yes, I see a number of bugs that amount to unhandled cases in the formatter's view of the syntax tree. We haven't tested it much on library-style verilog yet, but this makes for a nice sample set.

hzeller commented 4 years ago

Did another round of formatting: https://github.com/hzeller/XilinxUnisimLibrary/commit/20a71427fde8f60b3f2acc576121f94f4b4b360d

findings: 181 files out of 250 have some sort of parse error.

Funny: the Xilinx ascii-art, with a backslash at the end of a comment prompts verible to indent:

https://github.com/hzeller/XilinxUnisimLibrary/commit/20a71427fde8f60b3f2acc576121f94f4b4b360d#diff-6bd9051af528842b89d6bbf7973ba096

hzeller commented 4 years ago

Here is a breakdown of syntax errors seen. I suspect a lot of these are due to preprocessing weirdness.

      1 syntax error, rejected "always" (https://github.com/google/verible).
      1 syntax error, rejected ":" (https://github.com/google/verible).
      2 syntax error, rejected "buf" (https://github.com/google/verible).
      2 syntax error, rejected "endtask" (https://github.com/google/verible).
      4 syntax error, rejected "<" (https://github.com/google/verible).
      5 syntax error, rejected ";" (https://github.com/google/verible).
      6 syntax error, rejected "pulldown" (https://github.com/google/verible).
      7 syntax error, rejected "endmodule" (https://github.com/google/verible).
      9 syntax error, rejected "specify" (https://github.com/google/verible).
     10 syntax error, rejected "endspecify" (https://github.com/google/verible).
     13 syntax error, rejected "else" (https://github.com/google/verible).
     14 syntax error, rejected "`ifdef" (https://github.com/google/verible).
     24 syntax error, rejected "=" (https://github.com/google/verible).
     28 syntax error, rejected "`else" (https://github.com/google/verible).
     66 syntax error, rejected "<=" (https://github.com/google/verible).
    160 syntax error, rejected "$width" (https://github.com/google/verible).
    180 syntax error, rejected "$period" (https://github.com/google/verible).
    328 syntax error, rejected "end" (https://github.com/google/verible).
   4393 syntax error, rejected "(" (https://github.com/google/verible).
  10610 syntax error, rejected "$setuphold" (https://github.com/google/verible).
  12088 syntax error, rejected "assign" (https://github.com/google/verible).
mithro commented 4 years ago

@hzeller - A lot of those sound like the "specify" stuff which Yosys also doesn't like.

hzeller commented 4 years ago

Current status: formatted files in https://github.com/hzeller/XilinxUnisimLibrary/tree/formatting-experience3 branch (this commit )

165 files parse, 85 still fail (vs. 69 parse and 181 failure last time. Progress).

Unique syntax errors:

      1 syntax error, rejected "always" (https://github.com/google/verible).
      1 syntax error, rejected ":" (https://github.com/google/verible).
      2 syntax error, rejected "buf" (https://github.com/google/verible).
      2 syntax error, rejected "endtask" (https://github.com/google/verible).
      4 syntax error, rejected "<" (https://github.com/google/verible).
      5 syntax error, rejected ";" (https://github.com/google/verible).
      6 syntax error, rejected "pulldown" (https://github.com/google/verible).
      7 syntax error, rejected "endmodule" (https://github.com/google/verible).
      9 syntax error, rejected "specify" (https://github.com/google/verible).
     10 syntax error, rejected "endspecify" (https://github.com/google/verible).
     13 syntax error, rejected "else" (https://github.com/google/verible).
     14 syntax error, rejected "`ifdef" (https://github.com/google/verible).
     24 syntax error, rejected "=" (https://github.com/google/verible).
     28 syntax error, rejected "`else" (https://github.com/google/verible).
     66 syntax error, rejected "<=" (https://github.com/google/verible).
    160 syntax error, rejected "$width" (https://github.com/google/verible).
    180 syntax error, rejected "$period" (https://github.com/google/verible).
    328 syntax error, rejected "end" (https://github.com/google/verible).
   4393 syntax error, rejected "(" (https://github.com/google/verible).
  10610 syntax error, rejected "$setuphold" (https://github.com/google/verible).
  12088 syntax error, rejected "assign" (https://github.com/google/verible).
GitHub
hzeller/XilinxUnisimLibrary
Apache 2.0 licensed copy of the Xilinx Unisim library. - hzeller/XilinxUnisimLibrary
hzeller commented 4 years ago

I think a lot of errors also come from limited preprocessing by Verible and subsequent confusion.

hzeller commented 4 years ago

New run. Some formatting changes since last time (typically looks better):

https://github.com/hzeller/XilinxUnisimLibrary/commit/0ed1d382c81711edd9663c723d3e0edc15408b8b

145 files parse, 81 fail, and 24 crash formatting (b/169742626). Crashes are regression from last time:

    times | exit code | remark
----------+-----------+--------
      24  |   134     | crashes
      81  |     1     | parse failures
     145  |     0     | parses nicely
hzeller commented 4 years ago

Most notable changes:

became

 if (foo) bar;
 else if (baz) quux;

often this is better, sometimes not necessarily when the if expression is pretty large.

Concatenation operators became worse: (verilog/src/unisims/DSP_ALU.v)

  assign comux_w = ((smux & {comux4simd[46:0], 1'b0}) |
                    (wmux & {comux4simd[46:0], 1'b0}) |
                    (smux & wmux));

became:

  assign comux_w = ((smux & {
    comux4simd[46:0], 1'b0
  }) | (wmux & {
    comux4simd[46:0], 1'b0
  }) | (smux & wmux));
fangism commented 4 years ago

Most notable changes:

  • (good) A lot of $display("very long stuff", foo, bar) became broken to multiple lines.
  • (good) A lot of improvement in tabular aligning, in particular assignments.

if/else are more compact:

if (foo)
   bar;
else if (baz)
   quux;

became

 if (foo) bar;
 else if (baz) quux;

often this is better, sometimes not necessarily when the if expression is pretty large.

Concatenation operators became worse: (verilog/src/unisims/DSP_ALU.v)

  assign comux_w = ((smux & {comux4simd[46:0], 1'b0}) |
                    (wmux & {comux4simd[46:0], 1'b0}) |
                    (smux & wmux));

became:

  assign comux_w = ((smux & {
    comux4simd[46:0], 1'b0
  }) | (wmux & {
    comux4simd[46:0], 1'b0
  }) | (smux & wmux));

Acknowledged.

New run. Some formatting changes since last time (typically looks better):

hzeller@0ed1d38

145 files parse, 81 fail, and 24 crash formatting (b/169742626). Crashes are regression from last time:

    times | exit code | remark
----------+-----------+--------
      24  |   134     | crashes
      81  |     1     | parse failures
     145  |     0     | parses nicely

Acknowledged. If you have time, issues with reduced test cases would be helpful.