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

vcs: Ensure correct execution order of difftest DPI calls #563

Merged
merged 1 commit into from
Feb 8, 2025

Conversation

forever043
Copy link
Contributor

@forever043 forever043 commented Feb 6, 2025

An additional simv_step_event was introduced to control the execution order of the simv_nstep() DPI call. The order of DPI calls in different always blocks is inherently unpredictable. Since simv_nstep() depends on the state updated by other v_difftest_* DPI calls, a #0.1 delay was added to ensure that simv_nstep() is executed at the end of each clock posedge. This modification resolves potential timing issues caused by the implicit dependency between difftest DPI calls.

Additionally, the file extension of DifftestEndpoint.v was changed to .sv, as the event mechanism is a feature specific to SystemVerilog.

@forever043
Copy link
Contributor Author

we found the simv_nstep() was called before v_difftest_FpCSRState update dut->fcsr in our VCS testbench, which cause difftest check failed if instr commit update fcsr.

@forever043 forever043 changed the title vcs: Ensure correct execution order of difftest DPI calls wip: vcs: Ensure correct execution order of difftest DPI calls Feb 7, 2025
Copy link
Member

@poemonsense poemonsense left a comment

Choose a reason for hiding this comment

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

I remembered that we did discuss this issue before @klin02

Updates to DiffState should take effects before step. I think this was achieved by updating it at the previous clock cycle. Is it correct?

@klin02
Copy link
Member

klin02 commented Feb 7, 2025

https://github.com/OpenXiangShan/difftest/blob/master/src%2Fmain%2Fscala%2FGateway.scala#L242-L242

We have considered this issue before, see details above. We delay step signal to make sure simv_nstep called after dpic. Note this delay will only be applyed with GatewayEndpoint, which means we should also set difftest-config Z for vcs flow.

See more details in Difftest CI

@forever043 forever043 force-pushed the fix_vcs_nstep branch 3 times, most recently from fa5c88e to fd4bbcc Compare February 8, 2025 03:47
@forever043 forever043 changed the title wip: vcs: Ensure correct execution order of difftest DPI calls vcs: Ensure correct execution order of difftest DPI calls Feb 8, 2025
@forever043 forever043 changed the title vcs: Ensure correct execution order of difftest DPI calls WIP: vcs: Ensure correct execution order of difftest DPI calls Feb 8, 2025
@forever043 forever043 force-pushed the fix_vcs_nstep branch 2 times, most recently from 2ce67c5 to 7fe9977 Compare February 8, 2025 06:33
@forever043 forever043 changed the title WIP: vcs: Ensure correct execution order of difftest DPI calls vcs: Ensure correct execution order of difftest DPI calls Feb 8, 2025
@forever043 forever043 requested a review from klin02 February 8, 2025 06:33
klin02
klin02 previously approved these changes Feb 8, 2025
* for PALLADIUM platform it is possible that the difftest step
* was executed **BEFORE** all dffftest state update finished.
* (fortunately, it was not happened yet)
*/
Copy link
Member

Choose a reason for hiding this comment

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

In palladium, we will delay step signal to next cycle, to make sure simv_step trigger after other dpic. That need more trick to be correct, like a ping-pong buffer to handle delay-step and dpics at the next cycle coming together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

/*
 * In PALLADIUM, we delay the step signal to next cycle to make sure
 * `simv_step` was triggered after other DPI calls, which needs more
 * trick to be correct. Such as introducing `ping-pong buffer` to
 * handle the delay-step, and dpics at the next cycle coming together.
 */

An additional `simv_step_event` was introduced to control the execution
order of the `simv_nstep()` DPI call. The order of DPI calls in different
`always` blocks is inherently unpredictable. Since `simv_nstep()` depends
on the state updated by other `v_difftest_*` DPI calls, a `#0.1` delay was
added to ensure that `simv_nstep()` is executed at the end of each clock
posedge. This modification resolves potential timing issues caused by
the implicit dependency between difftest DPI calls.

Additionally, the file extension of `DifftestEndpoint.v` was changed to
`.sv`, as the `event` mechanism is a feature specific to SystemVerilog.

Signed-off-by: Jiuyue Ma <[email protected]>
@klin02 klin02 merged commit f17c1d8 into master Feb 8, 2025
5 checks passed
@klin02 klin02 deleted the fix_vcs_nstep branch February 8, 2025 09:14
klin02 added a commit to OpenXiangShan/XiangShan that referenced this pull request Feb 10, 2025
* commit: b537f528bbb9e400b9d0da8756219a5f6d107be9

Including:
* fix(xdma): remove replicate data parsing when USE_THREAD_MEMPOOL (OpenXiangShan/difftest#555)
* Preprocess: move from Gateway to seperate file (OpenXiangShan/difftest#559)
* Batch: collect from different groups in one cycle to reduce gates (OpenXiangShan/difftest#558)
* feat(query): support query for difftest-dpic data (OpenXiangShan/difftest#557)
* PerfCnt: count and sum for each DiffState when Batch (OpenXiangShan/difftest#560)
* Preprocess: skip loadEvent for single-core (OpenXiangShan/difftest#561)
* vcs: Refact DifftestEndpoint by split large always-block into piece (OpenXiangShan/difftest#565)
* vcs: Ensure correct execution order of difftest DPI calls (OpenXiangShan/difftest#563)
* Batch: disable split strategy for FPGA to reduce gates (OpenXiangShan/difftest#562)
* fix(elfloader): Sort phdr entries by paddr before return to readFromElf() (OpenXiangShan/difftest#566)
* Batch: rename BatchInterval to BatchStep, move to tail of StepInfo (OpenXiangShan/difftest#564)
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.

3 participants