[svsim] Add coverage collection to VCS backend #4793
Conversation
664def9 to
9265877
Compare
| // Note: This case class is being clever and re-using the names of the | ||
| // elements as the names of the options. This makes these very terse, but | ||
| // they then match, exactly, the documentation in the VCS manual. | ||
| final case class CoverageSettings( | ||
| line: Boolean = false, | ||
| cond: Boolean = false, | ||
| fsm: Boolean = false, | ||
| tgl: Boolean = false, | ||
| branch: Boolean = false, | ||
| assert: Boolean = false |
There was a problem hiding this comment.
Given in all likelihood there will be other options added, I would not make this a case class because then adding any argument breaks source (and binary) compatibility. Using a private case class is fine but I'd make the public API more explicit (which is annoying and more code but makes it much easier to evolve the API).
There was a problem hiding this comment.
This may be one situation where this is less risky as this is a direct translation of available VCS options. I also like the pattern here of being able to use case class to auto-derive Product and then use Product member functions to derive the compile flags.
What I will do is add all options that exist for VCS to this. However, given that this will only change if VCS changes, is it safe to use a case class?
Alternatively, I can implement Product manually.
WDYT?
There was a problem hiding this comment.
Okay that makes sense, let's keep it a case class--it's also always possible to manually update a case class if we ever really had to. SGTM 👍
| "-cm" | ||
| ) :+ coverageTypes.mkString("+") :+ | ||
| "+define+FIRRTL_BEFORE_INITIAL=// VCS coverage off" :+ | ||
| "+define+FIRRTL_AFTER_INITIAL=// VCS coverage on" :+ | ||
| "+define+USE_UNR_ONLY_CONSTRAINTS") ++ Option.when(line || tgl)("-cm_seqnoconst") ++ | ||
| Option.when(tgl)(Seq("-cm_tgl", "portsonly")).toSeq.flatten ++ | ||
| Option.when(branch)(Seq("-cm_branch", "ignoreMissingDefault")).toSeq.flatten |
There was a problem hiding this comment.
This confuses me a bit, so if tgl is true, we you get both: -cm +tgl and -cm_cgl portsonly?
There was a problem hiding this comment.
It confused me, too... However, this is the set of internal options.
How about this? I'll break these out into individual options and then bridge them internally.
There was a problem hiding this comment.
I backed this out and the options now are transparently mapped to their VCS options in a8b4257. I then added the toggle-specific options that are showing up here in 9559742.
A consideration is that we generally do not expose options this way for Chisel/firtool. Those, we happily make them Array[String] and let the user get it correct. With svsim, we already started down the path of type-safe options. Given the large amount of VCS options, we can either add these on demand as needed, or we could switch to the Array[String] approach for backend options.
There is some benefit to the Array[String] approach in that backends no longer have different option types. However, it's more stringy.
There was a problem hiding this comment.
I honestly would prefer less string-y arguments for Chisel and firtool, but am certainly not going to let the perfect be the enemy of the good on doing that for VCS coverage options and just leaving the Chisel and firtool options as Array[String] for the time being.
One tricky bit about structured types is that you probably want some form of parsing into your type which using Array[String] allows you to gloss over (this was a nice thing about how we/you did options annotations), but again, I still think this is good as is.
|
I will do a rebase and then land the three logical commits here individually. |
Add VCS backend options to enable coverage collection. The options for this are taken directly from our internal flow. I don't think that all these options need to be turned on (e.g., @girishpai indicated that `USE_UNR_ONLY_CONSTRAINTS` isn't load bearing). However, I'd rather migrate things wholesale and then refactor later. This includes a very simple test that a coverage database directory is created. This database is not created if coverage is turned off. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Add VCS backend options for generating `-cm_tgl` VCS options. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Add options to the VCS backend for controlling branch coverage. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
f742727 to
478655d
Compare
Add VCS backend options to enable coverage collection. The options for this are taken directly from our internal flow. I don't think that all these options need to be turned on (e.g., @girishpai indicated that
USE_UNR_ONLY_CONSTRAINTSisn't load bearing). However, I'd rather migrate things wholesale and then refactor later.This includes a very simple test that a coverage database directory is created. This database is not created if coverage is turned off.
CC: @andrew-gouldey-sifive
Release Notes
Add options to svsim VCS backend to allow for coverage collection.