ChiselSim peek/poke/expect for Enums, Records, Vecs, and generic Data #4824

kammoh · 2025-03-23T21:25:50Z

This PR changes the "testable" implicit classes in PeekPokeAPI to allow for a unified peek(), poke(), and expect API for all data types, including Records, Vecs, Enums, and nested aggregates.
This unified API would be very helpful when developing higher-level test abstractions, such as enqueue/deque over clocked Decoupled interfaces.

This is a step towards adding some useful features and ergonomic APIs similar to those provided by chiseltest (#4209 included a rough PoC for integrating some of these features).

The code does not look very pretty, but I could not figure out a cleaner way to make it work. Also, there is some repeated code.

I'd greatly appreciate comments and ideas for an alternative approach or other improvements.

Note: The previous (existing) implementation of expect[T]() seems incorrect. In addition to mistakingly using the same type name, T for the different method type parameter, the comparison uses object equality (!=).

def expect[T](
      expected:     T,
      encode:       (Simulation.Value) => T,
      buildMessage: (T, T) => String,
      sourceInfo:   SourceInfo
    ): Unit = {
...
        if (observed != expected) {

For the implemented Element values, the observed value is tuned into a Chisel object using the encode() method. Since the objects are not the same, the comparison would always fail, even when the value is expected. Seems the previous tests did not cover this, but using the tests in this PR, I get false failures with the previous expect + encode:

Observed value: 'UInt<33>(3521158774)'
Expected value: 'UInt<33>(3521158774)'

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

Feature

Desired Merge Strategy

Squash: The PR will be squashed and merged (choose this if you have no preference).

Release Notes

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.

jackkoenig

Overall I think this looks really good. Sorry for the delay on reviewing, I'm not super familiar with the PeekPokeAPI so had to look over it again so I could review 😅

I don't want to sandbag so feel free to push back, but my only real complaint is a nit that the tests are a big big rather than being split into small pieces for like each type being tested. Would it be too much trouble to split those up?

jackkoenig · 2025-04-23T05:02:56Z

src/test/scala-2/chiselTests/simulator/PeekPokeApiSpec.scala

+    it("should peek and poke various data types correctly") {
+      val numTests = 100
+      simulate(new TestPeekPokeEnum(w)) { dut =>
+        assert(w == dut.io.in.bits.a.getWidth)
+        val vecDim = dut.vecDim
+        val truncationMask = (BigInt(1) << w) - 1
+        for {
+          _ <- 0 until numTests
+          a = BigInt(w, rand)
+          b = BigInt(w, rand)
+          v1 = Seq.fill(vecDim)(BigInt(w, rand))
+          v2 = Seq.fill(vecDim)(BigInt(w, rand))
+          op <- TestOp.all
+        } {
+
+          dut.io.in.bits.poke(
+            chiselTypeOf(dut.io.in.bits).Lit(
+              _.a -> a.U,
+              _.b -> b.U,
+              _.v1 -> Vec.Lit(v1.map(_.U(w.W)): _*),
+              _.v2 -> Vec.Lit(v2.map(_.U(w.W)): _*)
+            )
+          )
+          dut.io.in.valid.poke(true)
+          dut.io.op.poke(op)
+          dut.clock.step()
+          dut.io.in.valid.poke(false)
+
+          val peekedOp = dut.io.op.peek()
+          assert(peekedOp.litValue == op.litValue)
+          assert(peekedOp.toString.contains(TestOp.getClass.getSimpleName.stripSuffix("$")))
+
+          val expected = op match {
+            case TestOp.Add => a + b
+            case TestOp.Sub => a - b
+            case TestOp.Mul => a * b
+            case _          => throw new Exception("Invalid operation")
+          }
+          val expectedCmp = a.compare(b) match {
+            case -1 => dut.CmpResult.LT
+            case 0  => dut.CmpResult.EQ
+            case 1  => dut.CmpResult.GT
+          }
+
+          dut.io.out.valid.expect(true.B)


Would it be possible to split this into a bunch of smaller tests of just individual types? Having the peeks and pokes of all types together kind of muddles what's being tested a bit.

I think the most reliable way to test peeks and pokes is to apply poke()s to the inputs, let the circuit do its thing, and then peek() at or expect() the outputs. I really think we need even more testing/coverage, not less.

I'm not saying have less testing or coverage, I'm just saying to do it in smaller chunks. For example, one could organize this by type and have basically a type parametered passthrough module (or have a register in there to delay a cycle), and then for each type and peek and poke method have a test that pokes the input and then checks the output.

My only real qualm is the "various" in it("should peek and poke various data types correctly"), I'd rather see:

it("should peek and poke Bool correctly") it("should peek and poke UInt correctly") it("should peek and poke ChiselEnum correctly") ... etc.

I'm not going to sandbag over this, I just think that would be better

Oh I see what you mean and do agree with you. I cleaned up the tests as much as I could. Please let me know if you think it still needs more cleanup.

src/test/scala-2/chiselTests/simulator/PeekPokeApiSpec.scala

kammoh · 2025-04-23T13:41:08Z

Hi @jackkoenig,
Thank you for the review and your feedback!
I'll try to cleanup and reorganize the tests, maybe break it up into multiple files.
I also have some doubts about what needs to be exposed as the public API at this point and also what needs to be supported for all types of Data. I'm also thinking about the usability and extensibility of the API for downstream libraries (e.g. FixedPoint and FloatingPoint data).
Also please let me know if you have any thoughts on the the expect functions that have a message generator argument.
I'm also testing this with my own libraries and implementations to make sure everything turns out consistent and works as expected.

kammoh · 2025-04-23T19:48:46Z

The other issue is the ability to extend for the multi-threaded version of PeekPokeAPI. It seems to me that the current structure makes code reuse difficult.

kammoh · 2025-04-24T03:06:24Z

All those .asInstanceOf[T] is scary and ugly, but I don't know how else we can do this.
Any thoughts @jackkoenig @seldridge ?

jackkoenig · 2025-04-24T21:59:23Z

src/main/scala/chisel3/simulator/PeekPokeAPI.scala

+        case (dat: Record, exp: Record) =>
+          new TestableRecord(dat).expect(exp, buildMsgFn _)
+        case (dat: Vec[_], exp: Vec[_]) =>
+          new TestableVec(dat).expectVec(
+            exp,
+            (obs: Data, _: Data, idx: Int) => {
+              require(obs.getClass == exp.getClass, s"Type mismatch: ${obs.getClass} != ${exp.getClass}")
+              buildMessage(obs.asInstanceOf[T], expected.asInstanceOf[T]) + s". First mismatch at index $idx."
+            }
+          )


I'm a little confused why there is special logic for Vec here but not for Record? I see Records are checked in the methods on TestableRecord so I think it's fine there's no extra checks, but Vecs are tested in TestableVec so why are there additional checks here?

It's probably not needed, but I wasn't 100% sure. The type generics for the extension classes differ between Vec and Record:

class TestableRecord[T <: Record](val data: T) // type parameter is the type itself, so methods accepting T are type-checked

vs

class TestableVec[T <: Data](val data: Vec[T]) extends PeekPokable[Vec[T]] // type parameter is the element type and is gone after type erasure

That's the only way I could get Vec[T] working. Please let me know if you think there's a cleaner way.

I cleaned up the code a little bit (using a weird workaround for Vec in _expect and _poke), so the special cases are now tucked away.

jackkoenig · 2025-04-24T22:01:54Z

src/main/scala/chisel3/simulator/PeekPokeAPI.scala

+        case x: Bool     => new TestableBool(x).peek().asInstanceOf[T]
+        case x: UInt     => new TestableUInt(x).peek().asInstanceOf[T]
+        case x: SInt     => new TestableSInt(x).peek().asInstanceOf[T]
+        case x: EnumType => new TestableEnum(x).peek().asInstanceOf[T]
+        case x: Record   => new TestableRecord(x).peek().asInstanceOf[T]
+        case x: Vec[_]   => new TestableVec(x).peek().asInstanceOf[T]


Are these the asInstanceOfs you're concerned about? I actually think there are fine--we know the type is correct because we have matched on the concrete runtime type, so barring a bug in TestableBool (or whatever for the given type), the cast will always work. The issue is just convincing the compiler that the type is correct--which it is.

Yes, my primary concern was really Vec, but I agree that all should be fine.

kammoh · 2025-04-25T20:48:32Z

src/main/scala/chisel3/simulator/PeekPokeAPI.scala

-      val peekedValue = peek()
+      // Not peeking the value beforehand or using `def` or `lazy val` results in a mysterious infinite recursion and StackOverflowError
+      // The value is not used if the expected value matches and incurs an overhead
+      // TODO: dig deeper amd see if we can avoid this


@jackkoenig Any thoughts on why this happens?

kammoh · 2025-04-25T20:49:33Z

src/main/scala/chisel3/simulator/PeekPokeAPI.scala

+
+      // Not peeking the value beforehand or using `def` or `lazy val` results in a mysterious infinite recursion and StackOverflowError
+      // The value is not used if the expected value matches and incurs an overhead
+      // TODO: dig deeper amd see if we can avoid this
      val peekedValue = data.peek()


be more flexible with the provided lit Vec

more concise, and matches the previous message tested in ChiselSimSpec

lengths are checked to match, elements are themselves checked when being individually poked

+ Refactor PeekPokeTestModule to its own file

- Use implicit functions inside the trait - Better error messages for expect on Vec

kammoh changed the title ~~peek/poke/expect for all Data types, include Records and Vecs~~ [chiselsim] Unified peek/poke/expect API for all Data types, include Records and Vecs Mar 24, 2025

kammoh force-pushed the peek_poke_data branch from da07ac7 to 6af1874 Compare March 26, 2025 02:30

kammoh force-pushed the peek_poke_data branch from 6af1874 to b0e523a Compare April 23, 2025 02:48

jackkoenig reviewed Apr 23, 2025

View reviewed changes

kammoh force-pushed the peek_poke_data branch from 346d63a to 0eb386a Compare April 23, 2025 20:21

jackkoenig reviewed Apr 24, 2025

View reviewed changes

kammoh force-pushed the peek_poke_data branch from e2406a3 to d98aebb Compare April 25, 2025 20:46

kammoh commented Apr 25, 2025

View reviewed changes

kammoh added 16 commits April 26, 2025 19:04

peek/poke/expect for all Data types, include Records and Vecs

5fc247d

cleanup

b1cf1cb

add peekBoolean for testableBool

2a221ab

remove type equivalency check for Vec

5014531

be more flexible with the provided lit Vec

fix test's expected message

39a861f

Change Bool expect fail message to 0/1 format

41ad37e

more concise, and matches the previous message tested in ChiselSimSpec

remove type equivalency check for Vec

380b0db

lengths are checked to match, elements are themselves checked when being individually poked

WIP: rethink and simplify API, make DRYer

9b3ab53

expect; peekValue should be public

c0bcbb6

fix to pass tests, default for package private + protected expect[U]

9e99923

Changed expect[U]; Fixed filename to match class case

c120166

move FailedExpectationException (and others) outside of PeekPokeAPI

4ac9e1b

+ Refactor PeekPokeTestModule to its own file

PeekPokeAPI companion object not an instance of trait

b7d5a68

- Use implicit functions inside the trait - Better error messages for expect on Vec

Vec expect message -> "First" mismatch

e85ec46

_poke for Vecs, rename _expect, mark as internal and package private

fd65700

reorganize + cleanup code and tests

dbb9393

kammoh force-pushed the peek_poke_data branch from d98aebb to dbb9393 Compare April 26, 2025 23:05

kammoh added 2 commits April 27, 2025 01:15

reduce the number of testcases

853bc25

nit picking messages; useless extra method

576c0f8

kammoh changed the title ~~[chiselsim] Unified peek/poke/expect API for all Data types, include Records and Vecs~~ ChiselSim peek/poke/expect for Enums, Records, Vecs, and generic Data Apr 27, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ChiselSim peek/poke/expect for Enums, Records, Vecs, and generic Data #4824

ChiselSim peek/poke/expect for Enums, Records, Vecs, and generic Data #4824

kammoh commented Mar 23, 2025 •

edited

Loading

jackkoenig left a comment

jackkoenig Apr 23, 2025

kammoh Apr 24, 2025

jackkoenig Apr 24, 2025

kammoh Apr 25, 2025

kammoh commented Apr 23, 2025

kammoh commented Apr 23, 2025

kammoh commented Apr 24, 2025

jackkoenig Apr 24, 2025

kammoh Apr 25, 2025

jackkoenig Apr 24, 2025

kammoh Apr 25, 2025

kammoh Apr 25, 2025

kammoh Apr 25, 2025

ChiselSim peek/poke/expect for Enums, Records, Vecs, and generic Data #4824

Are you sure you want to change the base?

ChiselSim peek/poke/expect for Enums, Records, Vecs, and generic Data #4824

Conversation

kammoh commented Mar 23, 2025 • edited Loading

Contributor Checklist

Type of Improvement

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

jackkoenig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kammoh commented Apr 23, 2025

kammoh commented Apr 23, 2025

kammoh commented Apr 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kammoh commented Mar 23, 2025 •

edited

Loading