CI workflow for SystemVerilog syntax checking #46
Conversation
…ons were expanded to match the tests and renamed.
Muxianesty
added
enhancement
.github/workflows/check_verilog.yml
Outdated
| @@ -0,0 +1,95 @@ | |||
| name: check-verilog | |||
There was a problem hiding this comment.
Why check-verilog name is used, if SystemVerilog description is tested?
.github/workflows/check_verilog.yml
Outdated
| MODULE_OUTPUT_PATH: "output.sv" | ||
| LIBRARY_OUTPUT_PATH: "lib.sv" |
There was a problem hiding this comment.
These environment variables are used at specific jobs/steps only. It would be better to declare them locally (well, as-local-as-possible).
.github/workflows/check_verilog.yml
Outdated
| LIBRARY_OUTPUT_PATH: "lib.sv" | ||
|
|
||
| jobs: | ||
| build-and-parse-verilog: |
There was a problem hiding this comment.
First of all, not "verilog" but "systemverilog", or, probably, "sv".
Also this job name looks a bit straightforward. What if one more step, (for example, logic synthesis) will be added here? What name will be used -- "build-and-parse-and-synthesize-systemverilog"? I'd use more general name like "synth-sv" -- our main task is to perform good HLS, and we can omit extra details here and write a describing comment, if needed,
.github/workflows/check_verilog.yml
Outdated
| strategy: | ||
| matrix: | ||
| kernel: [polynomial2, scalar3, matrixmul2, muxmul, addconst, movingsum] | ||
| config: [1] |
There was a problem hiding this comment.
What does it mean? Numbers aren't illustrative, by the way.
There was a problem hiding this comment.
Fixed with different configuration naming.
.github/workflows/check_verilog.yml
Outdated
| build-and-parse-verilog: | ||
| strategy: | ||
| matrix: | ||
| kernel: [polynomial2, scalar3, matrixmul2, muxmul, addconst, movingsum] |
There was a problem hiding this comment.
Probably a lexicographical order would be more deterministic for future kernels adding
README.md
Outdated
| @@ -276,7 +276,7 @@ For example, given subdirectory `polynomial2`, the compilation and execution com | |||
| ```bash | |||
| cmake -S . -B build -G Ninja -DCMAKE_PREFIX_PATH=~/firtool-1.72.0 -DSRC_FILES="~/utopia-hls/examples/polynomial2/polynomial2.cpp" | |||
| cmake --build build | |||
| build/src/umain hls --config examples/polynomial2/polynomial2.json -a --out-sv output | |||
| build/src/umain hls --config examples/polynomial2/polynomial2_1.json -a --out-sv output | |||
There was a problem hiding this comment.
What are these "_1" and "_2" filename suffixes for? Looks ugly.
.github/workflows/check_verilog.yml
Outdated
| matrix: | ||
| kernel: [addconst, matrixmul2, movingsum, muxmul, polynomial2, scalar3] | ||
| config: [STUB] | ||
| ## A workaround to intiailize an empty config array. |
.github/workflows/check_verilog.yml
Outdated
| - kernel: movingsum | ||
| config: add_int_2 | ||
| - kernel: movingsum | ||
| config: add_int_8 |
There was a problem hiding this comment.
Is it a good practice to enumerate different configs for same kernel in such way? Can't we do something like:
- kernel: somekernel
config: [config1, config2]
?
There was a problem hiding this comment.
Already tried it 1-2 commits before. Instead of what we want Github parses it as a JSON array of two elements.
This Pull Request introduces a new CI workflow, which uses Verilator to check generated SystemVerilog for syntax errors. Additionally, example configurations were expanded and renamed to match the ones used in tests.