[minor] Add simulation unit test construct#289
Conversation
fabianschuiki
force-pushed
the
fschuiki/simulation
branch
4 times, most recently
from
8eee3e1 to
968888e
Compare
seldridge
left a comment
seldridge
left a comment
There was a problem hiding this comment.
Very, very excited to see this!
| Time Step Clock Init | ||
| -------------- ----------- ----------- | ||
| 1 (optional) undefined undefined | ||
| 2 0 1 | ||
| 3 1 1 | ||
| 4 (optional) 1 1 or 0 | ||
| 5 (optional) 0 1 or 0 | ||
| 6 0 0 | ||
| 7 1 0 | ||
| 8 0 0 |
There was a problem hiding this comment.
Maybe it's time we pull in wavedrom waveforms. 🙃
There was a problem hiding this comment.
What is the reasoning behind making some of this optional? Would it be better to just make it exact?
There was a problem hiding this comment.
The optional stuff ensures that the simulator has a tiny bit of wiggle room in how it generates the waves. For example, the optional (1) allows for these two to be valid:
The optional (4) and (5) allow the init signal to drop anytime between the first and second rising edge:
I think I can collapse this to the following, for simplicity:
There was a problem hiding this comment.
Any chance you can integrate these with the spec? If not, can you drop the sources somewhere and I will integrate it.
spec.md
Outdated
| Additionally, the test may also declare a list of user-defined named parameters which can be integers, strings, arrays, or dictionaries. | ||
| These parameters are passed on to any tools that execute the tests. | ||
| No parameters with well-known semantics are defined yet. |
There was a problem hiding this comment.
I'm a little worried about how underspecified this part is.
There are a couple of ways to take it:
- Leave it as is and basically say that the simulation can have an arbitrary key-value mapping. I'd be hesitant to include anything beyond integers and strings first. (Also, if strings, what is the syntax? Double quoting strings like everything else?)
- Try to define this more strongly with an eye towards this being the eventual FIRRTL parameterization system.
- Go in a janky direction and say that there is a single string where you can put arguments and this is all implementation defined. (This is basically the same as (1), but is trying to go harder in the direction that this anything is allowed.)
I think I'm okay with (1), as it is. If you want dictionaries and lists, then that needs to be defined. Strings and ints only needs a little more text, but not much.
There was a problem hiding this comment.
I think we want this to be (1). This is what's already in the BNF syntax further down in the document, and what's implemented on the CIRCT side. I can add a few lines on how this works for the formal and simulation parts.
There was a problem hiding this comment.
I've added a separate "Test Parameters" section that elaborates for both formal and simulation what the syntax of the parameters is.
fabianschuiki
force-pushed
the
fschuiki/simulation
branch
2 times, most recently
from
332aa81 to
7574f9f
Compare
fabianschuiki
force-pushed
the
fschuiki/simulation
branch
from
7574f9f to
733cc84
Compare
|
Rebased this onto main with the 5.0.0 release. Any other thoughts on this @seldridge? |
|
|
||
| Name Type Direction | ||
| --------- ----------- ----------- | ||
| clock Clock input |
There was a problem hiding this comment.
Nit: FIRRTL syntax would be good here:
| clock Clock input | |
| clock `Clock`.{firrtl} input |
Ditto for the other ports.
| { decl_layer , newline } , | ||
| { decl_layer , newline } , |
There was a problem hiding this comment.
Thanks for this. Always feel free to land these changes directly.
Add a `simulation` unit test declaration analogous to the existing `formal` construct. This allows simulation test harness modules to be marked as entry points for simulation, which downstream tools can detect and run. This is more or less a copy of `formal`, with a few added constraints on the target module's ports and the exact simulation schedule.
fabianschuiki
force-pushed
the
fschuiki/simulation
branch
from
733cc84 to
b2fc892
Compare
2 participants
Add a
simulationunit test declaration analogous to the existingformalconstruct. This allows simulation test harness modules to be marked as entry points for simulation, which downstream tools can detect and run. This is more or less a copy offormal, with a few added constraints on the target module's ports and the exact simulation schedule.