Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modified testbench driver #6

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

mmrahorovic
Copy link
Collaborator

@mmrahorovic mmrahorovic commented Feb 22, 2024

Previously

  1. Input signals would be driven a delta cycle before the falling edge of the clock.
  2. List of inputs would be updated (i.e. a check for handshake) would occur a delta cycle before the falling edge, before toggling the clock for a full cycle.

Motivation for change

  1. Although functionally correct, it would be better to drive input signals a delta cycle after the rising edge of the clock. Besides making it arguably easier to read the waveform since input signal change at the rising edge, this behavior would be a more realistic simulation of the actual system/network since the input signal is assumed to come from a register from a preceding layer (and hence will only change a delta cycle after the rising edge).
  2. The RTL-MVU layer introduced in FINN, opposed to the HLS-based MVU layer, doesn't assert the input tready signal before the tvalid signal is high (they don't depend on each other though, which would be considered a wrong design, but the tready of one input signal depends on whether another input's tvalid is asserted). However, in the first cycle, a valid handshake would occur, despite the prediction based on a delta cycle before the falling edge that it wouldn't. Hence, the same input would be presented twice to the DUT, therefore consumed twice, and thus lead to a wrong output.

Changes introduced

  1. Drive input signals a delta cycle after the rising edge.
  2. Check for an input/output handshake a delta cycle before the rising edge.

Below is a screenshot of the new waveform.

waveform_im2

PR is marked as draft since testing is ongoing.

@mmrahorovic mmrahorovic mentioned this pull request Mar 6, 2024
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant