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

Fix to Support SYS Instructions with Register Rename #162

Merged
merged 10 commits into from
Apr 8, 2024

Conversation

ah-condor
Copy link
Contributor

@ah-condor ah-condor commented Mar 21, 2024

All instructions including SYS can possibly go through register rename. A problem occurs when a SYS instr gets a renamed dest register. SYS instrutions do not go in any execution pipe. In current code renamed registers can only be made ready in an execution pipe. So SYS instructions with dest register rename can never become ready. So any subsequent instruction waiting on the destination register just hangs. The solution I made was to give the ROB a scoreboard view and make the dest register ready when the ROB does the flush upon retiring SYS instructions. In an unrelated fix in the same block of code I also added a line to set the expected_flush_ variable to true or else the flush fails.

How was this tested:
make regress passed 72/72

ah-condor and others added 2 commits March 8, 2024 00:22
…me. A problem occurs when a SYS instr gets a renamed dest register. SYS instrutions do not go in any execution pipe. In current code renamed registers can only be made ready in an excution pipe. So SYS instructions with dest register rename can never become ready. So any subsequent instruction waiting on the destination register just hangs. The solution I made was to give the ROB a scoreboard view and make the dest register ready when the ROB does the flush upon retiring SYS instructions. In an unrelated fix in the same block of code I also added a line to set the expected_flush_ variable to true or else the flush fails.
core/ROB.cpp Outdated
// for SYS instr which doesn't have an exe pipe
void ROB::setup_scoreboard_view()
{
const char QNAME[] = "iq0";
Copy link
Collaborator

@aarongchan aarongchan Mar 21, 2024

Choose a reason for hiding this comment

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

This will error if the user renames the issue queues to something else such as "iq0_alu".

A suggested check could be to check if there was a renaming of issue queue units:

const auto exe_pipe_rename =
            olympia::coreutils::getPipeTopology(getContainer()->getRoot(), "exe_pipe_rename");
if (exe_pipe_rename.size() > 0)
{
       iq_name = exe_pipe_rename[0][1]; // just grab the first issue queue
}
else
{
      iq_name = "iq0"; # default name
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it and the following line of code

olympia::coreutils::getPipeTopology(getContainer()->getRoot(), "exe_pipe_rename");

throws an exception, somewhere inside the call getPipTopology, off the top of your head any idea why?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The issue is that getContainer()->getRoot() returns a RootTreeNode, while getPipeTopology() requires a TreeNode. A solution could be since we're in the ROB to just get the parent, which is cpu.core0 (from cpu.core0.ROB and then call the search for the issue_queue_rename parameter from there.

So change getContainer()->getRoot() to getContainer()->getParent()

@klingaard is there a more elegant way to approach this? My initial thoughts was to go from root to ensure we can find the parameter, but due to the TreeNode type requirement on getPipeTopology(), we would have to step down to core, which we could hard code to getRoot()->getChild("cpu.core0"). This could have issues in the future if Olympia is extended to multicore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok that works thanks Aaron

@klingaard
Copy link
Collaborator

Another method to fix this issue is to send the system instruction to an existing unit (say an ALU) and wait for it to be the oldest in the machine. The ROB has a port it uses to indicate the oldest instruction. When the system instruction is the oldest, it can execute and update the rename. This will prevent a flush for the CSR reads, which are costly.

core/ROB.hpp Outdated Show resolved Hide resolved
@ah-condor
Copy link
Contributor Author

ah-condor commented Mar 27, 2024

Another method to fix this issue is to send the system instruction to an existing unit (say an ALU) and wait for it to be the oldest in the machine. The ROB has a port it uses to indicate the oldest instruction. When the system instruction is the oldest, it can execute and update the rename. This will prevent a flush for the CSR reads, which are costly.

I am still thinking through what you described, but would you be ok with me merging this PR first and doing the optimized version later? I am not sure how long it will be if and when I do this alternate implementation.

Copy link
Collaborator

@klingaard klingaard left a comment

Choose a reason for hiding this comment

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

Yeah, we can move forward with this PR, but I would prefer to see the alternative CSR read that I proposed. Can you work on that in the background?

core/ROB.cpp Outdated

++num_flushes_;
break;
retireSysInst_(ex_inst_ptr);
Copy link
Collaborator

Choose a reason for hiding this comment

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

need 4 spaces...

@ah-condor
Copy link
Contributor Author

Yeah, we can move forward with this PR, but I would prefer to see the alternative CSR read that I proposed. Can you work on that in the background?

Yes Knute, I will be working on the CSR read implementation in the foreground. After discussion with my team. I may email you to discuss solutions.

@ah-condor
Copy link
Contributor Author

Ok I have made the last cosmetic change and sync'ed to master and ran regress. Let me know if there is anything else I need to do.

@klingaard klingaard merged commit 2ed051b into riscv-software-src:master Apr 8, 2024
4 checks passed
@ah-condor ah-condor mentioned this pull request May 20, 2024
klingaard pushed a commit that referenced this pull request Jun 5, 2024
This PR is a followup to
#162.

It is also discussed in:
#164
 
This PR changes the way SYS instructions are executed. Previously, SYS
instructions did not go through an execution pipe but instead were
dispatched to the ROB without delay, and the ROB handled the scoreboard
with respect to the destination register. This PR requires the SYS
instructions to pass through an execution pipe, thereby obviating any
special scoreboard code in the ROB.  With the exception of CSR read
instructions, the ROB will flush all SYS instructions.

This feature can be tested by running example_json.json workload, which
is part of the make regress suite.  Also, the sys execute pipe was added
to 3 config files big_core, medium_core and small_core.

How was this tested:
make regress passes 83/83.
 

---------

Co-authored-by: Anthony Hung <[email protected]>
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