Add execute flushing #137

danbone · 2024-01-04T17:09:06Z

Fix pipeline flush mechanisms by removing inflight instructions inside functional units, recovering the rename map, and adding trace rewind functionality
Add FlushCriteria class for requesting flushes to and from the FlushManager
Update FlushManager to handle and arbitrate between multiple flush sources
Add flush request port from ExecutePipe to FlushManager intended to be used for mispredicted branches
Delay completing instructions in ExecutePipe by a cycle to fix execute flush race conditions
Add branch identification methods for Inst
Add test to send random flushes from the branch unit

danbone · 2024-01-04T17:20:41Z

@knute-sifive few questions for you

For the flush testing mode we spoke about in the call yesterday. Were you thinking to use a parameter in the ExecutePipe?

I've run into a race condition where a branch mispredicts, completes and retires in the same cycle, before the flush is seen. Meaning any younger instruction that is retired alongside should have actually been flushed

See this log, where PID: 1975 is correctly refetched following the flush but also retired just before it.

{0000004137 00004137 top.cpu.core0.execute.br0 info} completeInst_: mispredicted branch uid: 2551  COMPLETED 113fe pid: 1974 'bne	x5,x7 +0xb2'  was actually not-taken
{0000004137 00004137 top.cpu.core0.rob info} retireInstructions_: retiring uid: 2551    RETIRED 113fe pid: 1974 'bne	x5,x7 +0xb2' 
{0000004137 00004137 top.cpu.core0.rob info} retireInstructions_: retiring uid: 2552    RETIRED 11402 pid: 1975 'bne	x12,x13 +0x3c' 
{0000004138 00004138 top.cpu.core0.rename info} getAckFromROB_: Retired instruction: uid: 2551    RETIRED 113fe pid: 1974 'bne	x5,x7 +0xb2' 
{0000004138 00004138 top.cpu.core0.rename info} getAckFromROB_: Retired instruction: uid: 2552    RETIRED 11402 pid: 1975 'bne	x12,x13 +0x3c' 
{0000004138 00004138 top.cpu.core0.rob info} handleFlush_: flushing uid: 2574 DISPATCHED 1144a pid: 1997 'slli	x14,x12, SHAMT=0x20'

I think we need to fix it so that we don't retire instructions that completed that cycle, and there's a few ways of doing this:
a. Add a sharedData object inside Inst that delays the completed status.
b. Add a completed event inside Inst and have retirement check if the event is scheduled before retiring (event should only be scheduled on the cycle it's completing)
c. Delay setting status to complete inside ExecutePipe and LSU

Any input?

danbone · 2024-01-05T17:10:03Z

As the race condition was between the flush coming from the execute and the instruction changing to completed status. I decided to separate changing the status to completed and the execution of the instruction.

klingaard · 2024-01-05T19:01:43Z

Thanks for doing this!

For the flush testing mode we spoke about in the call yesterday. Were you thinking to use a parameter in the ExecutePipe?

This is related to the "random" flushing idea (for testing), right? Here are my thoughts:

Since the flushes are instigated mostly (if not always) from the ROB, add the parameter there. It's simple integer-based parameter, enable_random_flushes that takes a random key. If non-zero, it's enabled with that key.
Add a few test with some random keys (maybe using the $RAMDOM variable from bash)

Glad you got the race condition rectified.

core/FlushManager.hpp

core/Inst.hpp

danbone · 2024-01-08T11:57:02Z

I hope I've address your comments above.

I played around with the ROB flushing but I found that doing it in the branch unit found more bugs. This should be a temporary thing to plug the gap until branch prediction is implemented.

For the ROB flushing, I could still put it in. What I did based on your comments was to use a hidden parameter that acted like a UID to trigger a flush from. Obviously this would only cause 1 flush per run, so it didn't have a lot of coverage compared to the branch unit flushing.

Signed-off-by: danbone <[email protected]>

danbone · 2024-01-10T15:28:09Z

Just noticed I forgot to change the FlushManager::FlushCriteria.flush method as requested

core/Rename.cpp

knute-mips

LGTM -- Aaron has a comment worth addressing.

knute-mips · 2024-01-10T19:53:41Z

core/ExecutePipe.cpp

+        // Testing mode to inject random branch misprediction to stress flushing mechanism
+        if (enable_random_misprediction_)
+        {
+            if (ex_inst->isBranch() && (std::rand() % 20) == 0)


This is awesome, but can a failure be reproduced? Probably should add a parameter to set a seed...

Also, can the 20 be parameterized as well? To increase the chances (for interesting experiments).

You could change the parameter enable_random_misprediction to random_misprediction_rate and default it to zero.

I forgot to reply back to this last night.

With std::rand, if the seed isn't set then it's always seeded with 1 according to google so should be reproducible (for now)

I think the random_misprediction_rate is a good idea. Something like tuning mispredictions per 1k branches.
I don't have a good knowledge of the C++ random libraries, but I saw I could use the discrete_distribution class. Happy to do this in another PR, as I think it'll need some back and forth discussion.

knute-mips · 2024-01-10T19:57:11Z

core/FlushManager.hpp

+                static const std::map<FlushCause, bool> inclusive_flush_map = {
+                    {FlushCause::TRAP,                 true},
+                    {FlushCause::MISFETCH,             true},
+                    {FlushCause::MISPREDICTION,        false},
+                    {FlushCause::TARGET_MISPREDICTION, false},
+                    {FlushCause::POST_SYNC,            false}
+                };


This is fine for now... but I wonder if this should be the decision of the instruction causing the flush...

Did think about doing it that way, but then I thought against it as I figured it would be more robust tying the cause and inclusiveness of the flush together. I don't feel too strongly about it, but perhaps we could keep it this way for now?

Yeah, I actually like this approach, so feel free to keep it this way for now.

knute-mips · 2024-01-10T19:57:57Z

core/Inst.cpp

+        is_call_(isCallInstruction(opcode_info)),
+        is_return_(isReturnInstruction(opcode_info)),


klingaard · 2024-01-10T22:29:46Z

Will merge this tomorrow morning CST.

kathlenehurt-sifive · 2024-01-11T15:04:37Z

core/ExecutePipe.cpp

-            else {
-                ++it;
+
+                ILOG("Flush Instruction ID: " << inst_ptr->getUniqueID() << " from issue queue");
            }


Is the issue queue age ordered? If so, can't we just break when we find the first instruction that is not included in the flush?

It should be, I avoided inferring the ordering though. I figured it's worth the extra cycles to make flushing more flexible. i.e. if we wanted to implement multi-threading.

Ah! I didn't think of that. Makes sense.

kathlenehurt-sifive · 2024-01-11T15:16:13Z

core/ExecutePipe.cpp

+    ////////////////////////////////////////////////////////////////////////////////
+
+    // Append instruction into issue queue
+    void ExecutePipe::appendIssueQueue_(const InstPtr & inst_ptr)


The Sparta Buffer class might be good to use for the issue queue. It allows for append and erase from anywhere in the buffer. It has its own implementation of the two methods you have here (appendIssueQueue_ and popIssueQueue_) so you wouldn't need to provide your own implementation.

Might be something @aarongchan can pick up in the issue queue changes

kathlenehurt-sifive · 2024-01-11T15:17:33Z

core/Fetch.cpp

+
+        auto flush_inst = criteria.getInstPtr();
+
+        // Rewind the tracefile


You can remove the if-statement here by passing !criteria.isInclusiveFlush() as the second parameter to reset instead.

kathlenehurt-sifive · 2024-01-11T15:34:12Z

Will merge this tomorrow morning CST.

None of my comments need to be addressed before merging. They can be resolved in another PR.

klingaard · 2024-01-11T18:21:39Z

Daniel, would you like more time to address Kathlene's comments or would you prefer to do that in another PR?

danbone · 2024-01-11T21:32:46Z

Another PR please

Add execute flushing

e4205f3

danbone marked this pull request as draft January 4, 2024 17:09

Daniel Bone added 4 commits January 4, 2024 19:52

Add isReturn/IsCall methods to Inst

aff4a0f

Add trace rewind to JSON InstGenerator

2d68761

separate execution and completion in executepipe

e51cefd

fix flush mechanisms in LSU

6302159

klingaard reviewed Jan 5, 2024

View reviewed changes

core/FlushManager.hpp Outdated Show resolved Hide resolved

core/FlushManager.hpp Outdated Show resolved Hide resolved

core/Inst.hpp Outdated Show resolved Hide resolved

core/Inst.hpp Outdated Show resolved Hide resolved

klingaard assigned danbone Jan 5, 2024

klingaard added the enhancement New feature or request label Jan 5, 2024

Daniel Bone added 4 commits January 5, 2024 22:48

Move is_branch/call/return/condbranch to constructor

6bc7cf6

Rename FlushEvent to FlushCause, add UNKOWN default

3d27352

Add branch misprediction test mode

66b358f

Clean up trace rewind to use templated method

873a971

danbone mentioned this pull request Jan 8, 2024

Handling flush in LSU updateIssuePriorityAfterCacheReload #136

Closed

danbone requested a review from knute-mips January 8, 2024 16:59

Use variant instead of tuple for rewind iterator

24042ba

danbone marked this pull request as ready for review January 8, 2024 18:31

danbone and others added 2 commits January 8, 2024 20:30

Merge branch 'master' into danbone/execute-flush

9f22089

Signed-off-by: danbone <[email protected]>

Rename flush criteria's flush method to includedInFlush

9c7a277

danbone requested a review from klingaard January 10, 2024 15:27

aarongchan reviewed Jan 10, 2024

View reviewed changes

core/Rename.cpp Outdated Show resolved Hide resolved

knute-mips approved these changes Jan 10, 2024

View reviewed changes

Remove dead code in rename

007cb42

klingaard approved these changes Jan 10, 2024

View reviewed changes

kathlenehurt-sifive reviewed Jan 11, 2024

View reviewed changes

klingaard merged commit ce29ae2 into riscv-software-src:master Jan 11, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add execute flushing #137

Add execute flushing #137

danbone commented Jan 4, 2024 •

edited

Loading

danbone commented Jan 4, 2024 •

edited

Loading

danbone commented Jan 5, 2024

klingaard commented Jan 5, 2024

danbone commented Jan 8, 2024

danbone commented Jan 10, 2024

knute-mips left a comment

knute-mips Jan 10, 2024

kathlenehurt-sifive Jan 11, 2024

danbone Jan 11, 2024

knute-mips Jan 10, 2024

danbone Jan 10, 2024

knute-mips Jan 10, 2024

knute-mips Jan 10, 2024

klingaard commented Jan 10, 2024

kathlenehurt-sifive Jan 11, 2024

danbone Jan 11, 2024

kathlenehurt-sifive Jan 11, 2024

kathlenehurt-sifive Jan 11, 2024

danbone Jan 11, 2024

kathlenehurt-sifive Jan 11, 2024

kathlenehurt-sifive commented Jan 11, 2024

klingaard commented Jan 11, 2024

danbone commented Jan 11, 2024

		is_call_(isCallInstruction(opcode_info)),
		is_return_(isReturnInstruction(opcode_info)),


		auto flush_inst = criteria.getInstPtr();

		// Rewind the tracefile

Add execute flushing #137

Add execute flushing #137

Conversation

danbone commented Jan 4, 2024 • edited Loading

danbone commented Jan 4, 2024 • edited Loading

danbone commented Jan 5, 2024

klingaard commented Jan 5, 2024

danbone commented Jan 8, 2024

danbone commented Jan 10, 2024

knute-mips left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klingaard commented Jan 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kathlenehurt-sifive commented Jan 11, 2024

klingaard commented Jan 11, 2024

danbone commented Jan 11, 2024

danbone commented Jan 4, 2024 •

edited

Loading

danbone commented Jan 4, 2024 •

edited

Loading