[chiselsim] Unified peek/poke/expect API for all Data types, include Records and Vecs #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.

jackkoenig · 2025-04-23T05:03:38Z

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

+          dut.io.in.bits.poke(
+            chiselTypeOf(dut.io.in.bits).Lit(
+              _.a -> 1.U,
+              _.b -> 2.U,
+              _.v1 -> Vec.Lit(Seq.fill(vecDim)(3.U(w.W)): _*),
+              _.v2 -> Vec.Lit(Seq.fill(vecDim)(4.U(w.W)): _*)
+            )
+          )
+          dut.io.in.valid.poke(true)
+          dut.io.op.poke(TestOp.Add)
+          dut.clock.step()
+
+          dut.io.out.bits.c.expect(5.U)


If this is just testing that the expectation is correct, could the rest of the test be simpler?

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.

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 · 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 ?

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 added 12 commits April 23, 2025 16:21

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

53b914a

cleanup

faaa865

add peekBoolean for testableBool

ec0e971

remove type equivalency check for Vec

db731fa

be more flexible with the provided lit Vec

fix test's expected message

93dedaa

Change Bool expect fail message to 0/1 format

88f97fb

more concise, and matches the previous message tested in ChiselSimSpec

remove type equivalency check for Vec

3f32551

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

WIP: rethink and simplify API, make DRYer

0e01a60

expect; peekValue should be public

03657c7

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

e5ed7a5

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

75e92f3

move FailedExpectationException (and others) outside of PeekPokeAPI

0eb386a

+ Refactor PeekPokeTestModule to its own file

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

PeekPokeAPI companion object not an instance of trait

a8a1c48

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

Vec expect message -> "First" mismatch

e2406a3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[chiselsim] Unified peek/poke/expect API for all Data types, include Records and Vecs #4824

[chiselsim] Unified peek/poke/expect API for all Data types, include Records and Vecs #4824

kammoh commented Mar 23, 2025 •

edited

Loading

jackkoenig left a comment

jackkoenig Apr 23, 2025

jackkoenig Apr 23, 2025

kammoh commented Apr 23, 2025

kammoh commented Apr 23, 2025

kammoh commented Apr 24, 2025

[chiselsim] Unified peek/poke/expect API for all Data types, include Records and Vecs #4824

Are you sure you want to change the base?

[chiselsim] Unified peek/poke/expect API for all Data types, include Records and Vecs #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

jackkoenig Apr 23, 2025

Choose a reason for hiding this comment

jackkoenig Apr 23, 2025

Choose a reason for hiding this comment

kammoh commented Apr 23, 2025

kammoh commented Apr 23, 2025

kammoh commented Apr 24, 2025

kammoh commented Mar 23, 2025 •

edited

Loading