SpinalHDL icon indicating copy to clipboard operation
SpinalHDL copied to clipboard

Bring the code coverage report to user's Spinal source code

Open numero-744 opened this issue 3 years ago • 8 comments

Hello,

Does SpinalHDL support test coverage? In the repository I've found the commit https://github.com/SpinalHDL/SpinalHDL/commit/33d9bcbeca3e6aed09a6483e201d06cc6b17551f which seems to add the feature but I couldn't find the documentation for it. I tried it and the coverage report is for the Verilog code. Do you think there is a way to retro-propagate the report to the SpinalHDL code?

numero-744 avatar Apr 23 '22 16:04 numero-744

So, for the SpinalHDL code coverage, there is two kinds :

  1. Scala runtime coverage, for this there is some scala tools which do it, Intellij does for instance.
  2. Backtracing verilog simulation coverage up to SpinalHDL scala code. => there is nothing like this so far. I personnaly haven't done coverage much, i just played once with it.

Also which kind of coverage are you aiming for ? Line execution ? or value coverage ?

Dolu1990 avatar Apr 25 '22 09:04 Dolu1990

I am aiming for toggle coverage in the designed component. If I correctly understand the documentation, it is not possible to run the design as-is (IIRC it states that SpinalHDL is not an HLS) so I have to simulate the elaborated design and get coverage over this.

A way to work would be to let the tool (e.g. Verilator) print a report with Verilog names and let the human/designer guess what values in its SpinalHDL/Scala code they represent. Do you think it would be suitable?

(Sorry for the delay I though I had replied to your message earlier)

numero-744 avatar May 01 '22 20:05 numero-744

(Sorry for the delay I though I had replied to your message earlier)

no worries ^^

If I correctly understand the documentation, it is not possible to run the design as-is (IIRC it states that SpinalHDL is not an HLS) so I have to simulate the elaborated design and get coverage over this.

Yes right

A way to work would be to let the tool (e.g. Verilator) print a report with Verilog names and let the human/designer guess what values in its SpinalHDL/Scala code they represent. Do you think it would be suitable?

That's what i would personnaly do, but to be honnest i'm never realy used any sort of coverage, so i'm not sure what verification people would exactly expect. (ex : aggregation of coverages comming from multiple tests, ... )

Then about " Verilog names and let the human/designer guess what values in its SpinalHDL/Scala code they represent". That's a important thing. So as long as the designer keep some discipline to keep signal named, i would say it is totaly ok. (https://spinalhdl.github.io/SpinalDoc-RTD/master/SpinalHDL/Structuring/naming.html?highlight=naming)

Dolu1990 avatar May 02 '22 10:05 Dolu1990

A problem for verification people would be signal name changes from one elaboration to the other. I think _zz_result is okay but when_Test_l117 seems to be an issue. Do you think it could be possible to compose names, at least for simple expressions? For instance:

when(value === 0) => wire when_value_eq_0;
when(!button) => wire when_not_button;

Maybe it could be extended in a recursive way to things such as:

when(a + b + c > 2) => when_a_plus_b_plus_c_gt_2

Verification people told me that modifying a part of the SpinalHDL code (well, any source for code generation) should change the smallest part of the Verilog code, and offsetting a line would change all when_ signals with the current implementation.

numero-744 avatar May 02 '22 12:05 numero-744

when_Test_l117 when_Test_l117 is extra coverage points that SpinalHDL generate. The equivalent hand written code in VHDL / Verilog would not show this kind of intermediate signal. So if the goal of the verification guy is to cover as much as a hand written VHDL / Verilog netlist, then they can just ignore all when_xxx_yyy, and it would provide the same coverage quality than what they are used to do (i assume) Else, if their goal is to always get 100 % coverage what ever they take as input, then yes i see the issue.

Do you think it could be possible to compose names, at least for simple expressions? For instance:

Yes it would be easy to implement. including the recursive version. It is just a bit of work to get all expression to provide a nice string, but totaly feasible

and offsetting a line would change all when_ signals with the current implementation.

Yes right. So the reason why it was implemented like this is that for synthesis it realy help tracing back a signal / critical path to the original scala code, as it will populate the netlist with a lot of helpfull names.

Something as when_a_plus_b_plus_c_gt_2 could be for sure implemented and be enabled as an option.

Dolu1990 avatar May 02 '22 17:05 Dolu1990

Making line-independent names as an elaboration option would be great!

numero-744 avatar May 02 '22 22:05 numero-744

Added a few options in dev (nameWhenByFile = false, inlineConditionalExpression = true) See the bottom of #575 This will inline conditions in the if statements, but has the bad side of eventualy duplicating code. (not as good as having a nice when_a_plus_b_plus_c_gt_2 style thing)

Dolu1990 avatar May 04 '22 15:05 Dolu1990

Thanks for the link, it makes me think to this:

class MyFsm extends Component {
  val io = new Bundle {
    val button = in Bool()
    val led = out Bool()
  }

  val fsm = new StateMachine() {
    val A : State = StateEntryPoint().whenIsActive ( when(io.button) ( goto(B) ) )
    val B : State = State().whenIsActive ( when(!io.button) ( goto(C) ) )
    val C : State = State().whenIsActive ( when(io.button) ( goto(D) ) )
    val D : State = State().whenIsActive ( when(!io.button) ( goto(A) ) )
    io.led := !isActive(A)
  }
}

It produces this:

  assign when_MyTopLevel_l145 = (! io_button);
  assign when_MyTopLevel_l147 = (! io_button);

So i think it would be great (but maybe harder to implement), with "expression-names" (just an idea of name for that feature), to be able to avoid duplication: naming one signal when_not_io_button instead of two signals with different names.

numero-744 avatar May 06 '22 09:05 numero-744

Related discussion: https://github.com/SpinalHDL/SpinalHDL/issues/783#issuecomment-1265938651

It is okay for me as it is now, I'm closing it. If needed, another issue can be opened.

numero-744 avatar Oct 03 '22 19:10 numero-744