riscv-software-src / riscv-perf-model

Example RISC-V Out-of-Order/Superscalar Processor Performance Core and MSS Model
Apache License 2.0
133 stars 56 forks source link

Add execute flushing #137

Closed danbone closed 8 months ago

danbone commented 9 months ago
danbone commented 9 months ago

@knute-sifive few questions for you

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

  2. 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 commented 9 months ago

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 commented 9 months ago

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:

  1. 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.
  2. Add a few test with some random keys (maybe using the $RAMDOM variable from bash)

Glad you got the race condition rectified.

danbone commented 9 months ago

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.

danbone commented 9 months ago

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

klingaard commented 8 months ago

Will merge this tomorrow morning CST.

kathlenehurt-sifive commented 8 months ago

Will merge this tomorrow morning CST.

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

klingaard commented 8 months ago

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

danbone commented 8 months ago

Another PR please