Add peek/poke/expect for memories

[nrother]

As requested in #145 a much needed feature (for me at least) would be peek/poke/expect support on memories.

This is currently fully supported in Treadle, but not exposed in the tester API.

I made a prototype implementation of a TestableMem and it seems to work well: https://gist.github.com/nrother/37693bb7df287f3ad9e528381c28855d

Maybe you could include test support for memories based on my prototype? If I'll find some time I'll try to convert it to a proper PR, but currently my understanding of the internals of Chisel and Treadle is quite limited. The code currently assumes that a Bundle always gets split into multiple memories, using an underscore as the separator. Maybe there is an API to query the memory name for a Bundle element?

[ducky64]

Cool!

So giving it a bit of though, there's more than one option for a memory access API:

• As you have in your PR, it would be mem.poke(i, value) and mem.peek(i). The implementation should be pretty straightforward, but it would be restricted in terms of working with entire memory "rows" - for example, if your memory is a Bundle, you can only poke and peek with Bundle literals.
• An alternative would be mem(i).poke(value). This is more in-line with how we can access sub-fields of Bundles, eg bundle.subfield.poke(). However, the implementation of this will be much less straightforward: mem is an object exists during hardware construction, and we can do an implicit-conversion to allow an indexed-poke, but mem(i) is a hardware construction operation and creates a new memory port. On the bright side, this API allows treating each "row" as a wire, eg mem(i).subfield.poke(value), but it's a lot less clear how to implement it, since hardware construction isn't permitted in the test environment.

In theory, the first API could allow sub-"row" pokes by taking in a partial bundle literals, and you can access the Bundle literal sub-field from a poke. Note that partial Bundle literal pokes, in general, are currently disallowed. In either case, sub-"row" pokes will require support in the simulator, or a shim read-modify-write operation in the interface layer.

Skimming through your PR, I think most of it looks reasonable, though integrating it into mainline code means that you can get direct access to the internals and avoid some of the boilerplate, though you'll also need to expand some internal interfaces to memory operations. But two things I'd probably change:

• I'm on the fence about whether to allow a bulk-memory-peek/poke operation, especially if the input length isn't checked to match the memory length.
• Need some way to refactor and dedup pokeSubElement with testableData.poke. Possibly a common utility function that serializes Data literals to BigInt? Should be a bit easier now that Bundle.litOption is in?

[nrother]

Some thoughts on this:

Note that partial Bundle literal pokes, in general, are currently disallowed.

Do you have any information on this? pokePartial looks like it is fully supported...

• I don't think that "sub-row-pokes" would require changes in the simulator: In the (Lo?)-FIRRTL the simulator works with, a bundle-memory gets decomposed to multiple individual memories, so updating only some of them should be easily possible.
• Therefore I don't think that Bundle.litOption would be a help here: A bundle must be poke into multiple memories, so having a BigInt of the whole Bundle is not much help...
• I think the bulk operations are a huge timesaver, since they allow you to easily prime the memory from a Seq (that is at least what I do with them)

I'll see if I can find some time to convert this sketch into an actual PR...

[ducky64]

About partial Bundle literal pokes: mainly an API design consideration, instead of a can-we-do-it issue (which we can, as demonstrated with pokePartial). The idea is that the common API is "strict", and if you want to do something a bit more loose, you need to explicitly say so by using a different, looser API.

That's an interesting observation on the Lo?Firrtl bundle memory transform - I'll ask at the next chisel-dev meeting about what we can rely on with regards to that behavior, and also what interfaces are common in simulators for memory access.

For bulk operations: I guess that makes sense. Main question would be what should happen when not all elements are passed in. I generally prefer a more explicit API, so maybe something like mem.pokeAll(...) and mem.pokeSome(...) (but with better naming)?

[ducky64]

We discussed this at the dev meeting today:

• In general, there are no guarantees on memory structure from FIRRTL transforms. The current behavior, defined by a transform, is to split memory-of-aggregates into one memory per "ground" (basic) type - this is what the gist is (I think?) coded for.
• There is also a SimplifyMems transform, which will pack aggregates into a single bitvector.
• The most general solution would be to support feedback from the FIRRTL transform, and use that to determine how to handle memory peek / poke operations. There is currently no support for this yet, and may need new APIs.
• In the short term, we should pick one and support it to get something working. It makes sense to support the currently observed behavior (memory-of-aggregates split into multiple memories), especially because SimplifyMems also doesn't support write masks.

For memory APIs (mem.pokeElement(i, value), mem(i).pokeElement(value)): also makes sense to have a similar philosophy: get something working so it can be useful. Given that, it probably makes sense to use the former mem-specific API. Also, while the latter is cleaner, very little code will probably actually need to access memory elements like wires, and it will be difficult to implement.

[nrother]

Just to confirm: The gist currently assumes the observed behavior that bundle-mems are split to multiple "basic-type" mems, with the names split by _.

The "done is better than perfect" approach sounds good for me!

[ducky64]

Yes, I think you're correct. I'm not completely sure on naming guarantees, but in general it's probably best to use a "feedback" API (eg, a rename map from FIRRTL - instead of a hard-coded naming structure and assuming it will hold) where possible.

[nrother]

Digging this up, after #358 has landed.

Is there any possibility to implement this easily now? I tried to update my code above to use the correct type SimulatorContext (Since ThreadleContext is now private) but it is missing the expectMemory function.

Before I resort to even more reflection-hacking I just wanted to ask if it became possible to implement this cleanly now 🙂

[ekiwi]

The only way to access the expectMemory functionality for now is through the old PeekPokeTester compatibility API (in chiseltest.iotesters).
My minimum requirement for exposing functionality in the mainstream API is that it needs to work for Treadle and for Verilator. People should be able to seamlessly switch between the two backends.

So if someone from the community implements peekMemory and pokeMemory support for Verilator, I would be happy to expose it. The way to go about implementing support for Verilator is to add some peekMemory/pokeMemory tests to this suite of tests: https://github.com/ucb-bar/chiseltest/blob/master/src/test/scala/chiseltest/simulator/MemoryCompliance.scala
These tests should pass for treadle (check by running the TreadleMemoryCompliance tests), but will fail for Verilator (check by running the VerilatorMemoryCompliance tests). Then you can start adding the required changes to the Verilator backend. You will probably have to touch at least https://github.com/ucb-bar/chiseltest/blob/master/src/main/scala/chiseltest/simulator/VerilatorSimulator.scala and the files in https://github.com/ucb-bar/chiseltest/tree/master/src/main/scala/chiseltest/simulator/jna.

[nrother]

Hm, ok. I personally would love to see peek/pokeMem support in the API, even if it would only work on one backend. Verilator support would be awesome, of course. Maybe I can have a look into it (thanks for the pointers!)

For now, I updaded the Gist liked above to work with Chiseltest 0.5.0

[nrother]

I just had a quick look into the Verilator API and found no evidence on how to peek/poke memories... For me this looks like being totally unsupported in Verilator :/

[ekiwi]

I just had a quick look into the Verilator API and found no evidence on how to peek/poke memories... For me this looks like being totally unsupported in Verilator :/

It can be done, but is fairly tricky to achieve (cocotb afaik allows poking/peeking memory with Verilator). However, I think I would be OK adding some memory peek/poke methods that will throw an exception unless you are using treadle. If anyone wants to open a PR with a minimal number of methods.