[svsim] Fix bugs in generated simulation code for Verilator backend. #5054

xiaoh105 · 2025-10-11T05:13:43Z

Rewrite the run_simulation function in simulation_driver.cpp to handle delayed events in BlackBox modules properly. Now run_simulation will check whether there're pending delayed events, and wake up these pending events by calling the eval function at correct timepoint to allow the delay scheduler schedule these events.
Also add a test in ChiSimSpec.scala to demonstrate the case.

Contributor Checklist

Did you add Scaladoc to every public function/method?
Did you add at least one test demonstrating the PR?
Did you delete any extraneous printlns/debugging code?
Did you specify the type of improvement?
Did you add appropriate documentation in docs/src?
Did you request a desired merge strategy?
Did you add text to be included in the Release Notes for this change?

Type of Improvement

Bugfix

Desired Merge Strategy

Squash

Release Notes

Fix the run_simulation API in resource file simulation-driver.cpp for verilator backend to allow delayed events from BlackBox be scheduled by Verilator properly.

Reviewer Checklist (only modified by reviewer)

Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
Did you review?
Did you check whether all relevant Contributor checkboxes have been checked?
Did you do one of the following when ready to merge:
- Squash: You/ the contributor Enable auto-merge (squash) and clean up the commit message.
- Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

Rewrite the run_simulation function in simulation_driver.cpp to handle delayed events in BlackBox modules properly. Add a test checking in ChiSimSpec.scala to demonstrate the case.

linux-foundation-easycla · 2025-10-11T05:13:52Z

The committers listed above are authorized under a signed CLA.

✅ login: xiaoh105 / name: xiaoh (28dedcd, 14f3c44, 1a58d26, 6d1ea52)

seldridge

Generally LGTM. Thanks for the PR!

Nitty comments throughout. I am running this on our internal CI as I want to make sure it doesn't break anything. We (obviously) aren't exercising this code path as I've never seen this error come up before. I do want to make sure there are no regressions, though.

seldridge · 2025-10-11T19:53:05Z

src/test/scala-2/chiselTests/simulator/scalatest/ChiselSimSpec.scala

+            new svsim.verilator.Backend.CompilationSettings(
+              traceStyle = conf.traceStyle,
+              outputSplit = conf.outputSplit,
+              outputSplitCFuncs = conf.outputSplitCFuncs,
+              disabledWarnings = conf.disabledWarnings,
+              disableFatalExitOnWarnings = true,
+              enableAllAssertions = conf.enableAllAssertions,
+              timing = Some(svsim.verilator.Backend.CompilationSettings.Timing.TimingEnabled)
+            )


This constructor is now deprecated to avoid problems with binary compatibility and changes to this case class. The way to do this now is:

Suggested change

new svsim.verilator.Backend.CompilationSettings(

traceStyle = conf.traceStyle,

outputSplit = conf.outputSplit,

outputSplitCFuncs = conf.outputSplitCFuncs,

disabledWarnings = conf.disabledWarnings,

disableFatalExitOnWarnings = true,

enableAllAssertions = conf.enableAllAssertions,

timing = Some(svsim.verilator.Backend.CompilationSettings.Timing.TimingEnabled)

)

svsim.verilator.Backend.CompilationSettings.default

.withDisableFatalExitOnWarnings(true)

.withTiming(Some(svsim.verilator.Backend.CompilationSettings.Timing.TimingEnabled))

seldridge · 2025-10-11T19:55:24Z

svsim/src/main/resources/simulation-driver.cpp

+    uint64_t stepDuration =
+      std::min(static_cast<uint64_t>(delay), testbench->nextTimeSlot() - context->time());


This static_cast from signed to unsigned looks dangerous. Can the PR include an assert that delay is positive? I'm also slightly nervous that delay could go negative inside this while loop. It may make sense to check it every iteration. WDYT?

Unfortunately, Verilog DPI doesn't have an unsigned type that I am aware of, so the signature of run_simulation can't be changed.

Actually this while loop is executed only when delay > 0. So if the delay becomes negative it will automatically break the loop.
But I think your worry is valid. So I'll add an assertion at the beginning of the function(to make sure the input delay is non-negative), and one after the loop exits(to check whether delay goes negative after the while loop). This can guarantee that delay won't turn negative throughout the function. Do you think this is reasonable ?

seldridge · 2025-10-11T19:56:38Z

svsim/src/main/resources/simulation-driver.cpp

+      if (*finish) {
+        return;
+      }


Nit:

Suggested change

if (*finish) {

return;

}

if (*finish)

return;

seldridge · 2025-10-11T19:57:41Z

svsim/src/main/resources/simulation-driver.cpp

+  if (delay > 0) {
+    context->timeInc(delay);
+  }


Nit:

Suggested change

if (delay > 0) {

context->timeInc(delay);

}

if (delay > 0)

context->timeInc(delay);

seldridge · 2025-10-11T19:58:36Z

src/test/scala-2/chiselTests/simulator/scalatest/ChiselSimSpec.scala

    it("should dump a VCD waveform when traceStyle is Vcd and enableWaves is used") {

-      implicit val vaerilator = verilatorWithVcd
+      implicit val verilator = verilatorWithVcd


Thanks for fixing this!

seldridge · 2025-10-11T20:13:31Z

src/test/scala-2/chiselTests/simulator/scalatest/ChiselSimSpec.scala

+
+        // Some simple logic using the clock
+        val counter = RegInit(0.U(8.W))
+        counter := counter + 1.U


Nit:

Suggested change

counter := counter + 1.U

counter :<= counter + 1.U

seldridge · 2025-10-11T20:31:15Z

svsim/src/main/resources/simulation-driver.cpp

+    if (stepDuration <= 0) {
+      failWithError("Found event that should be sheduled ealier.");
+    }


Nit throughout: no need for braces around single statements following if statements:

Suggested change

if (stepDuration <= 0) {

failWithError("Found event that should be sheduled ealier.");

}

if (stepDuration <= 0)

failWithError("Found event that should be sheduled ealier.");

seldridge · 2025-10-11T20:40:02Z

svsim/src/main/resources/simulation-driver.cpp

+      testbench->eval();
+      *finish = context->gotFinish();
+      if (*finish) {
+        return;
+      }


This code snippet appears twice. Consider moving this to a static function/lambda.

seldridge · 2025-10-11T20:42:50Z

svsim/src/main/resources/simulation-driver.cpp

+  if (delay > 0) {
+    context->timeInc(delay);
+  }
 }


I think this makes sense. I was trying to see if there was some way to combine this with the earlier testbench-eval() to make this a do-while loop. However, there is always going to need to be some optional execution after stepDuration is subtracted.

seldridge · 2025-10-11T20:47:13Z

src/test/scala-2/chiselTests/simulator/scalatest/ChiselSimSpec.scala

+        dut.io.delayedInitial.expect(0.U)
+        dut.clock.step(1, 1000)
+
+        dut.io.delayedInitial.expect(1.U)
+        dut.clock.step(1, 1000)
+
+        dut.io.in.poke(0.U)


Please include .expect of delayedIn as well.

xiaoh105 · 2025-10-12T08:01:58Z

@seldridge Thank you for your detailed comments and suggestions. This is actually my first pr to an open-source project so I learned a lot from these comments.
I've added another commit to the pull request which fixed all the problems listed in the comments. Could you please check it again for other possible format problems and run your internal CI again (if needed) ?

seldridge

LGTM

Edit: I did not see any internal failures with this change.

seldridge · 2025-10-13T14:53:48Z

src/test/scala-2/chiselTests/simulator/scalatest/ChiselSimSpec.scala

+        delayed.io.in :<= io.in
+        io.delayedIn :<= delayed.io.delayedIn
+        io.delayedInitial :<= delayed.io.delayedInitial


The explicit assignment is fine. You may consider:

Suggested change

delayed.io.in :<= io.in

io.delayedIn :<= delayed.io.delayedIn

io.delayedInitial :<= delayed.io.delayedInitial

delayed.io :<>= io

I tried delayed.io :<>= io but it doesn't compile on my machine. The error is:

[411] [error] src/test/scala-2/chiselTests/simulator/scalatest/ChiselSimSpec.scala 499:20: Connection between sink (Delayed.delayedInitial: IO[UInt<1>]) and source (Foo.io.delayedInitial: IO[UInt<1>]) failed @: delayedInitial in Delayed cannot be written from module Foo. [411] [error] delayed.io :<>= io [411] [error] ^ [411] [error] src/test/scala-2/chiselTests/simulator/scalatest/ChiselSimSpec.scala 499:20: Connection between sink (Delayed.delayedIn: IO[UInt<1>]) and source (Foo.io.delayedIn: IO[UInt<1>]) failed @: delayedIn in Delayed cannot be written from module Foo. [411] [error] delayed.io :<>= io [411] [error] ^ [411] [error] src/test/scala-2/chiselTests/simulator/scalatest/ChiselSimSpec.scala 499:20: Connection between sink (Foo.io.in: IO[UInt<1>]) and source (Delayed.in: IO[UInt<1>]) failed @: io.in in Foo cannot be written from module Delayed. [411] [error] delayed.io :<>= io [411] [error] ^ [411] [error] There were 3 error(s) during hardware elaboration.

I'm not sure whether it's my environment's problem or there's something wrong with the expression.
Edit: I found the fix. Revert the expression to io :<>= delayed.io and it works.

It may be that they need to be reversed. However, I've had the same issues with these operations and just wind up writing it manually. The PR is fine. No changes needed.

Just now I've found the correct way. Using io :<>= delayed.io and it works.

seldridge · 2025-10-13T14:57:26Z

Let me know when you're ready with this and I can land it.

xiaoh105 · 2025-10-13T16:36:47Z

Fixed and ready to go. Thanks for your review !

fix(svsim): Rewrite generated simulation code for Verilator backend.

1a58d26

Rewrite the run_simulation function in simulation_driver.cpp to handle delayed events in BlackBox modules properly. Add a test checking in ChiSimSpec.scala to demonstrate the case.

xiaoh105 mentioned this pull request Oct 11, 2025

Verilator reports error when using ChiselSim while BlackBox modules contain initial block #5052

Closed

seldridge added the Bugfix Fixes a bug, will be included in release notes label Oct 11, 2025

seldridge reviewed Oct 11, 2025

View reviewed changes

fix(svsim): code format on ChiselSimSpec and simulation-driver.cpp.

6d1ea52

seldridge approved these changes Oct 13, 2025

View reviewed changes

fix(svsim): code format on ChiselSimSpec and simulation-driver.cpp.

14f3c44

seldridge approved these changes Oct 13, 2025

View reviewed changes

seldridge enabled auto-merge (squash) October 13, 2025 16:45

Merge branch 'main' into main

28dedcd

seldridge disabled auto-merge October 13, 2025 17:09

seldridge enabled auto-merge (squash) October 13, 2025 17:09

seldridge merged commit c06598f into chipsalliance:main Oct 13, 2025
16 checks passed

		uint64_t stepDuration =
		std::min(static_cast<uint64_t>(delay), testbench->nextTimeSlot() - context->time());

[svsim] Fix bugs in generated simulation code for Verilator backend. #5054

[svsim] Fix bugs in generated simulation code for Verilator backend. #5054

Uh oh!

Conversation

xiaoh105 commented Oct 11, 2025

Contributor Checklist

Type of Improvement

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

Uh oh!

linux-foundation-easycla bot commented Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seldridge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiaoh105 commented Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seldridge left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiaoh105 Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seldridge commented Oct 13, 2025

Uh oh!

xiaoh105 commented Oct 13, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

linux-foundation-easycla bot commented Oct 11, 2025 •

edited

Loading

xiaoh105 commented Oct 12, 2025 •

edited

Loading

seldridge left a comment •

edited

Loading

xiaoh105 Oct 13, 2025 •

edited

Loading