vproc / vicuna

RISC-V Zve32x Vector Coprocessor
Other
162 stars 47 forks source link

Result transaction id and data issue (when `result_ready` has random delay) #82

Closed moimfeld closed 2 years ago

moimfeld commented 2 years ago

Hi @michael-platzer,

Issue

I found that sometimes, for example while executing the vloxei8_8.S program, Vicuna has two result transactions for one id (and for another id there is no transaction). This issue only happens when the random result transaction delay is activated in the UVM environment. I did not spend a lot of time understanding what could be the problem but from what I have seen, the issue might be that Vicuna updates the result.id too early, i.e. if there are outstanding result transactions it updates the result.id to the value of the next transaction while the handshake of the current result transaction is going on.

In the screenshot you can see at 465 ns, at the beginning of the ready valid handshake, that the result.id value is updated from 4 to 6. And to my understanding the result.id should have stayed 4 during this handshake.

issue_82

The issue can be reproduced by running the cvxif_test_direct_issue_82 test with the UVM environment.

michael-platzer commented 2 years ago

Hi @moimfeld,

I believe this issue is fixed now. The source of the problem was that the CSR result buffer within the result module was overwritten even when the module was not ready to accept a new CSR result.

moimfeld commented 2 years ago

Hi @michael-platzer,

I just ran the cvxif_test_direct_issue_82, and the problem is indeed fixed. Thanks a lot!

moimfeld commented 2 years ago

Hi @michael-platzer

I reopen this issue because I just found a related issue that did not get addressed by 21f25d07759fd850ad1fdb101f44ce83b5107a70. Not only is the id updated too early but also the result (of course the id issue was addressed with 21f25d07759fd850ad1fdb101f44ce83b5107a70). From checking the signals in the vproc_result module I think the issue comes from line 186.

https://github.com/vproc/vicuna/blob/917ee6975c6c0b810ce743142ea1682aa977df90/rtl/vproc_result.sv#L186

You can see in the screenshot below, that result_csr_delayed_q is high during the problematic result transaction. And therefore the result_csr_data_delayed_i is assigned to the xif_result_if.result.data (and result_csr_data_delayed_i itself is updated too early).

The UVM environment therefore prints the following warning and error message:

# UVM_WARNING /csem/divm/users/mid/workspace/core-v-wrapper/vicuna/uvm/sim/../src/generic/env/cvxif_scoreboard.svh(228) @ 830: uvm_test_top.env.scoreboard [CVXIF_SCOREBOARD] Instr "vsetvli t0, t0, e8, m1, tu, mu" with id = 4 in Prog "vloxei8_8" failed: 
# at        830 ns: 
# 
# Result Difference
# reference.rd_data: 0000000f, result_table[ 4].result_item.data: 00000010
# 
# UVM_ERROR /csem/divm/users/mid/workspace/core-v-wrapper/vicuna/uvm/sim/../src/generic/env/cvxif_scoreboard.svh(252) @ 1220: uvm_test_top.env.scoreboard [CVXIF_SCOREBOARD] Program "vloxei8_8" failed

issue_82_2

You can reproduce this issue by running the cvxif_test_direct_issue_82_2 test.

Sidenote: I did not discover this issue earlier because there was a bug in the verification environment that suppressed result comparisons.

michael-platzer commented 2 years ago

Hi @moimfeld,

sorry, forgot about that, should be fixed now in d3c1cf8.

moimfeld commented 2 years ago

Thank you very much! The issue is fixed now 😄