Skip to content
This repository has been archived by the owner on Aug 19, 2024. It is now read-only.

Delay first rising clock edge by 1 half-period in VPI Harness #591

Merged
merged 2 commits into from
Jan 9, 2023

Conversation

jackkoenig
Copy link
Collaborator

This prevents a race condition between the rising clock edge and randomization when RANDOMIZE_DELAY=1. Note that the traditional Chisel randomization delay of #0.002 is based on VCS's default timescale of 1ns/1ps and does not properly work with other timescales. It is much safer to use a full timescale delay, and thus prudent to delay the first rising clock edge till the 2nd time step.

Note that this does change waveforms slightly such that you have a full clock period with no clock before it starts rather than the first rising edge being after merely half a clock-period. See old and new (except of course there is no zero-time falling edge, I can't figure out how to get rid of it in Wavedrom)

Screen Shot 2022-12-22 at 2 37 45 PM

This prevents a race condition between the rising clock edge and
randomization when RANDOMIZE_DELAY=1. Note that the traditional Chisel
randomization delay of #0.002 is based on VCS's default timescale of
1ns/1ps and does not properly work with other timescales. It is much
safer to use a full timescale delay, and thus prudent to delay the first
rising clock edge till the 2nd time step.
Copy link
Collaborator

@ekiwi ekiwi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot this yesterday, but this PR is missing a test that fails without your fix. Could you please add an Icarus Verilog based test that fails without the delay? Otherwise we might at some point wonder why the delay is there, remove it, see that no tests are failing and thus believe that it isn't actually necessary.

@jackkoenig
Copy link
Collaborator Author

Could you please add an Icarus Verilog based test that fails without the delay?

Yep, good point and suggestion. I have not quite gotten a test working yet, but pushed a WIP branch that I'll look at again next week or in the new year.

@aswaterman
Copy link
Member

Isn't the failing test going to be nondeterministic (at least with respect to Verilog codegen changes), since the resolution of the race depends on the event queue ordering?

@ekiwi
Copy link
Collaborator

ekiwi commented Jan 7, 2023

Let's just get it in, even without a test.

@ekiwi ekiwi added this to the 0.5.x milestone Jan 7, 2023
@ekiwi ekiwi added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Jan 7, 2023
@ekiwi ekiwi merged commit 54d0cf9 into main Jan 9, 2023
mergify bot pushed a commit that referenced this pull request Jan 9, 2023
This prevents a race condition between the rising clock edge and
randomization when RANDOMIZE_DELAY=1. Note that the traditional Chisel
randomization delay of #0.002 is based on VCS's default timescale of
1ns/1ps and does not properly work with other timescales. It is much
safer to use a full timescale delay, and thus prudent to delay the first
rising clock edge till the 2nd time step.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 54d0cf9)
@mergify mergify bot added the Backported This PR has been backported to marked stable branch label Jan 9, 2023
mergify bot added a commit that referenced this pull request Jan 9, 2023
…600)

This prevents a race condition between the rising clock edge and
randomization when RANDOMIZE_DELAY=1. Note that the traditional Chisel
randomization delay of #0.002 is based on VCS's default timescale of
1ns/1ps and does not properly work with other timescales. It is much
safer to use a full timescale delay, and thus prudent to delay the first
rising clock edge till the 2nd time step.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 54d0cf9)

Co-authored-by: Jack Koenig <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Backported This PR has been backported to marked stable branch Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants