adding a Plusargs Bridge, with unit tests and TutorialSuite tests #1291

sifive-benjamin-morse · 2022-11-03T01:16:31Z

Adding a Plusargs Brige. Currently only the %d format is supported.

Related PRs / Issues

UI / API Impact

Verilog / AGFI Compatibility

This shouldn't change any existing code, but adds compatibilty with

Contributor Checklist

Is this PR's title suitable for inclusion in the changelog and have you added a changelog:<topic> label?
Did you add Scaladoc/docstring/doxygen to every public function/method?
Did you add at least one test demonstrating the PR?
Did you delete any extraneous prints/debugging code?
Did you state the UI / API impact?
Did you specify the Verilog / AGFI compatibility impact?

If applicable, did you regenerate and publicly share default AGFIs?

If applicable, did you apply the ci:fpga-deploy label?

If applicable, did you apply the Please Backport label?

Reviewer Checklist (only modified by reviewer)

Note: to run CI on PRs from forks, comment @Mergifyio copy main and manage the change from the new PR.

Is the title suitable for inclusion in the changelog and does the PR have a changelog:<topic> label?
Did you mark the proper release milestone?
Did you check whether all relevant Contributor checkboxes have been checked?

…ala docs and doxygen

nandor · 2022-11-04T05:31:54Z

sim/midas/src/main/cc/bridges/plusargs.cc

+  std::string name = name_orig;
+
+  // remove all leading white space
+  while (name.at(0) == ' ') {


Breaking from convention, would it be possible to do such argument parsing outside the bridge constructor? Not necessarily in this PR, but maybe in the future. If exceptions will be disabled, there will be no way to report errors from constructors without exiting right away.

Yeah, that makes a ton of sense. The whole +arg managements needs a rework (better scoping, better discoverability etc...) and pulling the parsing out of the bridge constructor would play nicely into that.

sim/midas/src/main/cc/bridges/plusargs.cc

sim/src/main/cc/midasexamples/PlusargsModule.h

sim/src/test/scala/midasexamples/TutorialSuite.scala

davidbiancolin

The scala bridge code looks great -- i mostly have nits. My main concern is with the ScalaTests for the target-side of the bridge which could be cleaned up some and made dry-er.

sim/midas/src/main/cc/bridges/plusargs.h

sim/midas/src/main/cc/bridges/plusargs.cc

sim/src/main/cc/midasexamples/PlusargsModule.h

sim/midas/src/main/scala/midas/widgets/PlusargsBridge.scala

sim/midas/src/test/scala/midas/widgets/PlusargsBridgeSpec.scala

sim/src/test/scala/midasexamples/TutorialSuite.scala

Co-authored-by: David Biancolin <david.biancolin@sifive.com>

…dge.scala Co-authored-by: David Biancolin <david.biancolin@sifive.com>

Co-authored-by: David Biancolin <david.biancolin@sifive.com>

davidbiancolin

This is really shaping up!

Mostly just nits now

sim/midas/src/main/scala/midas/widgets/PlusArgsBridge.scala

davidbiancolin · 2022-11-16T23:17:36Z

sim/midas/src/test/scala/midas/core/SimUtilsSpec.scala

@@ -67,7 +74,7 @@ class SimUtilsSpec extends AnyFlatSpec {
        portIntRef.copy(component = Seq(TargetToken.Field("bits")))
      ),
    )
-    require(data.typeEquivalent(UInt(32.W)))
+    require(DataMirror.checkTypeEquivalence(data, UInt(32.W)))


Why did you have to change these? I feel like nandor got them already?

I changed these by grabbing from nandor's commit directly because his commit was waiting to be merge to master. I could have waited and rebased instead

If you want, I can rebase before we merge. However because I pulled the files exactly the squash merge should identify the files as same and not generate any conflicts

sim/midas/src/test/scala/midas/widgets/PlusArgsBridgeSpec.scala

davidbiancolin · 2022-11-16T23:25:08Z

sim/src/main/scala/midasexamples/PlusArgsModule.scala

+  */
+class PlusArgsDUT(implicit val p: Parameters) extends Module {
+
+  def testGroup0(): PlusArgsBridgeParams = {


In the future you could use an enum-like thing to capture more semantic information about each test in the type itself.

e.g.:

sealed trait PlusArgTest { def params: PlusArgsBridgeParams} case object WidePlusArgTest extends PlusArgTest { def params = PlusArgsBridgeParams(name = "plusar_v=%d", default = BigInt("276783146634859761135"), width = 68)} ... case object PlusArgsTestTypeKey extends Field[PlusArgTest]

etc...

sim/src/main/scala/midasexamples/PlusArgsModule.scala

sim/midas/src/main/scala/midas/widgets/PlusArgsBridge.scala

Co-authored-by: David Biancolin <david.biancolin@sifive.com>

davidbiancolin

One final thing -- you probably want to add a few instantiations to firesim_top.cc

Otherwise ship it!

sifive-benjamin-morse added the changelog:added Put PR title in 'Added' section of changelog label Nov 3, 2022

sifive-benjamin-morse requested review from davidbiancolin and t14916 November 3, 2022 01:16

adding a Plusargs Bridge, with unit tests and TutorialSuite tests, sc…

6c708e9

…ala docs and doxygen

sifive-benjamin-morse force-pushed the bmorse/plusargs-squash branch from 1572c59 to 6c708e9 Compare November 4, 2022 04:59

sifive-benjamin-morse requested a review from nandor November 4, 2022 05:19

nandor reviewed Nov 4, 2022

View reviewed changes

davidbiancolin reviewed Nov 8, 2022

View reviewed changes

Benjamin Morse and others added 14 commits November 8, 2022 22:28

reduce verilator runs from 4 to 2 for Plusargs tests

2fbe964

spelling sim/midas/src/main/cc/bridges/plusargs.h

94a92f8

Co-authored-by: David Biancolin <david.biancolin@sifive.com>

[skip ci] spelling sim/midas/src/main/cc/bridges/plusargs.h

f83b0da

Co-authored-by: David Biancolin <david.biancolin@sifive.com>

[skip ci] spelling sim/midas/src/main/scala/midas/widgets/PlusargsBri…

6021c8e

…dge.scala Co-authored-by: David Biancolin <david.biancolin@sifive.com>

remove parens

9983f9b

Co-authored-by: David Biancolin <david.biancolin@sifive.com>

grab CI fix from 5d14f6f

56bc660

take this file from 6a3f0fd to fix tests

9d85cd7

[skip ci] comment only

ad1a744

Co-authored-by: David Biancolin <david.biancolin@sifive.com>

give the scala test groups specific names

2a52392

move require into case class

7619906

cleanup spec, and test all apply() methods under object PlusargsBridge

37f6992

Address most of the cpp comments

1a4b02d

change Plusargs to PlusArgs everywhere except filenames

82da487

rename files to fix capitalization

5e9d803

davidbiancolin reviewed Nov 16, 2022

View reviewed changes

Benjamin Morse and others added 2 commits November 17, 2022 02:03

address comments, adding Key, elaborateModule

4ae812f

fix tabulate x2 and semicolon

0c204f7

Co-authored-by: David Biancolin <david.biancolin@sifive.com>

davidbiancolin approved these changes Nov 18, 2022

View reviewed changes

t14916 approved these changes Nov 18, 2022

View reviewed changes

Benjamin Morse added 3 commits November 18, 2022 19:18

fix tabulate

feee3f0

add a few instantiations to firesim_top.cc

9dcf779

fix format

99947fd

try clang-format under ubuntu

4ed3c4e

sifive-benjamin-morse merged commit 7aa7ee2 into main Nov 21, 2022

sifive-benjamin-morse deleted the bmorse/plusargs-squash branch November 21, 2022 20:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding a Plusargs Bridge, with unit tests and TutorialSuite tests #1291

adding a Plusargs Bridge, with unit tests and TutorialSuite tests #1291

sifive-benjamin-morse commented Nov 3, 2022 •

edited

nandor Nov 4, 2022

davidbiancolin Nov 4, 2022

davidbiancolin left a comment

davidbiancolin left a comment

davidbiancolin Nov 16, 2022

sifive-benjamin-morse Nov 17, 2022

sifive-benjamin-morse Nov 17, 2022

davidbiancolin Nov 16, 2022

davidbiancolin left a comment

adding a Plusargs Bridge, with unit tests and TutorialSuite tests #1291

adding a Plusargs Bridge, with unit tests and TutorialSuite tests #1291

Conversation

sifive-benjamin-morse commented Nov 3, 2022 • edited

Related PRs / Issues

UI / API Impact

Verilog / AGFI Compatibility

Contributor Checklist

Reviewer Checklist (only modified by reviewer)

nandor Nov 4, 2022

Choose a reason for hiding this comment

davidbiancolin Nov 4, 2022

Choose a reason for hiding this comment

davidbiancolin left a comment

Choose a reason for hiding this comment

davidbiancolin left a comment

Choose a reason for hiding this comment

davidbiancolin Nov 16, 2022

Choose a reason for hiding this comment

sifive-benjamin-morse Nov 17, 2022

Choose a reason for hiding this comment

sifive-benjamin-morse Nov 17, 2022

Choose a reason for hiding this comment

davidbiancolin Nov 16, 2022

Choose a reason for hiding this comment

davidbiancolin left a comment

Choose a reason for hiding this comment

sifive-benjamin-morse commented Nov 3, 2022 •

edited