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

Question about s2_mq_nack Signal of LoadPipe.scala #3916

Closed
5 tasks done
diantaowang opened this issue Nov 22, 2024 · 1 comment · Fixed by #3936
Closed
5 tasks done

Question about s2_mq_nack Signal of LoadPipe.scala #3916

diantaowang opened this issue Nov 22, 2024 · 1 comment · Fixed by #3936
Assignees
Labels
question Question requiring answer

Comments

@diantaowang
Copy link

Before start

  • I have read the RISC-V ISA Manual and this is not a RISC-V ISA question. 我已经阅读过 RISC-V 指令集手册,这不是一个指令集相关的问题。
  • I have read the XiangShan Documents. 我已经阅读过香山文档。
  • I have searched the previous issues and did not find anything relevant. 我已经搜索过之前的 issue,并没有找到相关的。
  • I have searched the previous discussions and did not find anything relevant. 我已经搜索过之前的 discussions,并没有找到相关的。
  • I have reviewed the commit messages from the relevant commit history. 我已经浏览过相关的提交历史和提交信息。

Describe the question

When I read the source code and try to figure out the reasons which make load to replay, I encountered some problems.

version: master, 6639e9a (2024-11-21)

The following situations will cause the load instruction to be quickly reissued in the S3 stage:

  • DCache bank conflict
  • way predict error
  • mshr is full when cahce miss
  • write back queue reject the miss request
  • miss queue cancel the miss request

But the logic of generating s2_mq_nack signal is puzzling. The original code snippet as follows:

io.lsu.s2_mq_nack       := (resp.bits.miss && (!s2_miss_req_fire || s2_nack_no_mshr || io.mq_enq_cancel || io.wbq_block_miss_req))

Why we need s2_miss_req_fire? It's confusing that s2_valid==0 will lead s2_miss_req_fire==0 and s2_mq_nack==1. It seems like that s2_valid is directly related to s2_mq_nack. This may or may not cause errors, due to the complex prioritization logic that follows. For example, the tlb miss will lead the s2_valid equals 0, but C_TM has higher priority than C_DR.

I would like to know whether s2_miss_req_fire is redundant. If not, please explain why, and if so, please modify the code for better readability.

Thanks!!!

@diantaowang diantaowang added the question Question requiring answer label Nov 22, 2024
@Anzooooo Anzooooo self-assigned this Nov 25, 2024
@Anzooooo
Copy link
Member

Thank you for your interest in the OpenXiangShan.
It does look like this is a redundant signal, which may have been inherited from the code of a previous version. We will discuss again whether more redundant signals can be removed, and we will get back to you if there is any new development.
Thank you again.

linjuanZ pushed a commit that referenced this issue Nov 27, 2024
Remove the redundant logic of the miss queue nack based on the issues
raised in the
issues(#3916).

When a tlb miss and a dcache miss occur at the same time and the miss
queue nack, it will cause the `LoadUnit` to generate both replay signals
`C_TM` and `C_DR`. We will give priority to `C_TM`, which is why we need
to send a kill signal to dcache when a tlb miss occurs.

Although there was no problem before, as the
issue(#3916) says, this
will cause ambiguity, and the miss queue nack message is already
included in `s2_nack_no_mshr`, so the choice is to remove the
`s2_miss_req_fire` signal from the generation logic of the `s2_mq_nack`
signal.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Question requiring answer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants