FuseSoC and Github Linter Action

[wallento]

This adds a Github action that will automatically check pushes to the repository and pull requests. It will fail when the Verilator linter issues any warning. I propose to use this to have committers either suppress the warnings or add a meaningful message in the waiver file. You can find a writeup about waiver files in Verilator here: https://wallento.cs.hm.edu/post/20200612-verilator-waivers/ A demo of what the Linter Action produces can be found here for the ibex core, the clou are the source annotations which make it really powerful:

• https://github.com/wallento/ibex/pull/4/commits/bbf8e108bb313655544ebb490ff9816ffb981b87
• https://github.com/wallento/ibex/pull/5/

The waiver file looks ugly now, but I see a good path forward solving or waiving the warnings correctly, happy to chat about it. You don't need to do anything except merging, Actions are a builtin.

[olofk]

This looks great and will be very useful to keep up code quality. Hopefully we cam reduce the number of existing warnings too. There are quite a few of them but EH1 was verilator clean IIRC so it should be doable

[wallento]

Yeah, I skipped through them. My gut feeling is that half of them are solved easily within an hour (hidden names, widths, etc.)

[agrobman]

We use warning disable switches, see tools/Makefile

[wallento]

That is not what this PR is about, to be frank. At the moment you are suppressing warnings at simulation build. A linter flow will help improve the code quality and not suppress warnings, this is just to kick start a new flow. Plus this PR adds a CI check for new code and code proposals not to add new Linter failures.

[agrobman]

Our development/production simulator is xcelium. It compiles our design with no warnings. Compilations with VCS/Questa have no warnings too. Verilator is the only one, having warnings, because is not fully compliant with SV standard. Our design also passed standard lint tools checks, like spyglass and was synthesized with industry leading synthesis tools.
Our goal was to allow simulations of our cores with a free simulation tool – the Verilator. It took quit an effort to make it work. We have no plans to change design sources just to comply 100% with the Verilator deficiencies. Hopefully Verilator will evolve and thus won't warn legitimate code...

[wallento]

Sorry, @agrobman, but you clearly miss the point. It has nothing to do with SV compliance. If you have any problem with the synthesizable part of SV not working with Verilator, I encourage you to open an issue with Verilator, it will probably be resolved quickly. Both Swerv and Verilator are part of CHIPS Alliance, so there should be a mutual interest here. So, you say none of the linting tools gives you any warnings. Did you actually check the output of Verilator? Just some random picks out of the 514 warnings:

%Warning-UNUSED: ../src/chipsalliance.org_cores_SweRV_EL2_0/design/ifu/el2_ifu.sv:255:10: Signal is not driven, nor used: 'ifc_fetch_req_f_raw'
                                                                                        : ... In instance el2_swerv_wrapper.swerv.ifu
  255 |    logic ifc_fetch_req_f_raw;

The signal is defined and not used. Why define it? What purpose does it serve? Spyglass doesn't mention it?

%Warning-VARHIDDEN: ../src/chipsalliance.org_cores_SweRV_EL2_0/design/ifu/el2_ifu_bp_ctl.sv:648:18: Declaration of signal hides declaration in upper scope: 'j'
  648 |         for (int j=0; j< LRU_SIZE; j++) begin
      |                  ^
                    ../src/chipsalliance.org_cores_SweRV_EL2_0/design/ifu/el2_ifu_bp_ctl.sv:309:15: ... Location of original declaration
  309 |    genvar     j, i;
      |               ^

Is it really a great idea to use the same identifier again?

%Warning-WIDTH: ../src/chipsalliance.org_cores_SweRV_EL2_0/design/lib/el2_lib.sv:39:15: Bit extraction of var[9:2] requires 4 bit index, not 2 bits.
                                                                                      : ... In instance el2_swerv_wrapper.swerv.ifu.bp.f1hash
   39 |    assign hash[pt.BTB_ADDR_HI:pt.BTB_ADDR_LO] = pc[pt.BTB_INDEX1_HI:pt.BTB_INDEX1_LO] ^
      |          

Is the variable actually defined correctly?

So, to come to an end, I genuinely don't care. This is just a service. If you don't believe in style linting at all or have your demo run without trashing the terminal with 514 warnings, fine. Feel free to close the issue, I have no intention to lecture you about code style. But please try to better understand what Verilator Linter warnings are, I am pretty confident that the majority of the warnings are very legitimate. "Necessary" for "industry-grade" cores? Not sure. But "Hopefully Verilator will evolve and thus won't warn legitimate code..." is an entirely incorrect characterization of the "problem".

[olofk]

I see two benefits with this patch.

1. Currently there are so many warnings so that there is no choice but to turn them off, which is always a bad thing. This brings a handy list to go through. Most of these are probably harmless and can be waived, but just like @wallento I noticed undriven signals and width mismatches that can definitely cause problems. This is an opportunity to increase code quality

2. This currently waives the current list of warnings. Going forward, this means that any new warnings would be spotted immediately which is a safeguard against decreases in code quality

[aprnath]

Hi Stefan and Olof, Thanks for PR and the comments/justifications. Agreed that an itemized waiver list is better than blanket disabling of linter checks. We'll discuss ramifications and respond.

Thank you.

[agrobman]

@wallento,

What verilator version are you using?

[wallento]

@aprnath thanks, just drop me a mail if you have any questions or want to discuss it in general. @agrobman Verilator 4.036. It is based on our docker image (2020.06-rc2, to be released soon): https://github.com/librecores/ci-docker-image

[wallento]

From https://github.com/verilator/verilator/issues/2433 where @agrobman asked:

I don't understand what do you want.

You started all this complaining that you got a lot of warnings and we don't run lint. I explained what we don't see any warnings and why. I didn't say if I want to use verilator lint or not. I knew nothing about verilator lint at time of our release. I know that commercial simulators issue errors and warnings. Our check in policy is "no simulator warnings". And we followed it.

I guess -Wno-WIDTH is less masking lint problems than -Wno-lint, so we used this one. BTW, we consulted with Winston at release time how to suppress these warnings and we followed his advice.

You can't complain that we used older tool version 3 months ago, than one you use now and we even don't know exists.

How do you want us to handle your PR? with what verilator version? Is the latest clean from "bogus" width warnings? If not, when will we get "clean" one?

I think those questions are mostly answered in the commits and above. The path forward is simple:

1. You want this push and PR linter checks and think they are good and want to make sure what you are doing on a warning-by-warning basis and gradually wipe the waiver file: merge now.
2. You think its generally a good idea and are happy to get such contributions, but don't feel its worth the effort now: keep it open with adding an acceptance criteria for another round of dicussions.
3. You think the Verilator linter is bad and your workflow and the project will not benefit from it whatsoever: Close it, but your learning could be that -Wno-lint is your "friend".

My suggestion is 2.

[agrobman]

I agree with 2) too. Let us know good verilator version to start with.

[wallento]

When anyone from your team finds time and patience, I encourage you to post WIDTH warnings here that you believe are entirely wrong or way too pedantic. I will go through them too, but some prioritization of reviewing them is helpful.

[agrobman]

the big one is sized parameters, I mentioned in verilator post. Before I ask designers to look in the warnings I need 'fixed' verilator version. I'd asked them to do this already, but got push back with all these bogus warnings ...

Looking at your paper, I think, I don't like waver solutions for us. Our design evolves /changes too often - line based waiver is too much maintenance. (Is waiver line number based?) I prefer no warnings by construction solution or lint comments for us.

BTW, will lint disable comments work with -f verilog option?

if compilation files list contains ... disable_lint_file design_file1 ... design_fileN enable_lint_file ...

where "disable_lint_file" and "enable_lint_file" contain lint disable and enable comments?

to disable linting in part of the design files without touching them...