[MCA] Support in-order CPUs with MicroOpBufferSize=1

[andreadb]

Hi Jay,

is this patch needed because otherwise you see the following crash/assertion failure?

llvm-mca: /home/andrea/llvm-project/llvm/lib/MCA/HardwareUnits/RetireControlUnit.cpp:50: unsigned int llvm::mca::RetireControlUnit::dispatch(const llvm::mca::InstRef&): Assertion `(AvailableEntries >= Entries) && "Reorder Buffer unavailable!"' failed.

I had a better look at the implementation of our new InOrderIssueStage, and I am not convinced that we do the right checks for the reorder buffer.

TL;DR:
Your patch has the effect of disabling the reorder buffer logic entirely. RCU queries would simply be ignored, and an invalid "retire token ID" would be associated to all instructions.
This all makes sense to me, since the end result is that we want to bypass the RCU. So the patch LGTM.

That being said, this patch forced me to have another look at our implementation of the in-order pipeline, and I think I found a few problems in how we simulate the RCU for in-order processors. I have also reported a more general issue in PR41796.

The retire stage was originally added by Andrew to deal with processors like Cortex-A55 which - under specific conditions - allow OoO execution of some instructions. Quoting a document which I found on the web "While the Cortex-A55 core only issues instructions in-order, due to the number of cycles required to complete more complex floating-point and NEON instructions, out-of-order retire is allowed on ..." some instructions.

I think that the logic in the new InOrderIssue stage is missing some important checks. In particular, we are definitely missing a check on the availability of the reorder buffer. If we have run out of entries, then the issue should be stalled. While this check is not important for your target, it is important for cortex-a55 (because it declares a ROB with multiple entries in the "extra processor info" struct). That check should be ignored for other targets, for which we don't care about simulating a ROB.

[foad]

is this patch needed because otherwise you see the following crash/assertion failure?

Yes, that was the failure, and I just did the minimum work I could to make MicroOpBufferSize=1 work like MicroOpBufferSize=0. I don't really know what reorder buffers and retire control units are -- they are not part of my mental model of how the AMDGPU processors work.

[andreadb]

In D98356#2617708, @foad wrote:

is this patch needed because otherwise you see the following crash/assertion failure?

Yes, that was the failure, and I just did the minimum work I could to make MicroOpBufferSize=1 work like MicroOpBufferSize=0. I don't really know what reorder buffers and retire control units are -- they are not part of my mental model of how the AMDGPU processors work.

Fair enough :).
Your patch looks good. Thanks!

llvm/lib/MCA/HardwareUnits/RetireControlUnit.cpp

 
 #define DEBUG_TYPE "llvm-mca"
 
 namespace llvm {
 namespace mca {
 
 RetireControlUnit::RetireControlUnit(const MCSchedModel &SM)
     : NextAvailableSlotIdx(0), CurrentInstructionSlotIdx(0),
-      NumROBEntries(SM.MicroOpBufferSize),
-      AvailableEntries(SM.MicroOpBufferSize), MaxRetirePerCycle(0) {
+      AvailableEntries(SM.isOutOfOrder() ? SM.MicroOpBufferSize : 0),
+      MaxRetirePerCycle(0) {
   // Check if the scheduling model provides extra information about the machine
   // processor. If so, then use that information to set the reorder buffer size
   // and the maximum number of instructions retired per cycle.
   if (SM.hasExtraProcessorInfo()) {
     const MCExtraProcessorInfo &EPI = SM.getExtraProcessorInfo();
     if (EPI.ReorderBufferSize)
       AvailableEntries = EPI.ReorderBufferSize;
     MaxRetirePerCycle = EPI.MaxRetirePerCycle;
   }
   NumROBEntries = AvailableEntries;
-  bool IsOutOfOrder = SM.MicroOpBufferSize;
-  if (!IsOutOfOrder && !NumROBEntries)
+  if (!SM.isOutOfOrder() && !NumROBEntries)
     return;
   assert(NumROBEntries && "Invalid reorder buffer size!");
   Queue.resize(2 * NumROBEntries);
 }
 
 // Reserves a number of slots, and returns a new token.
 unsigned RetireControlUnit::dispatch(const InstRef &IR) {
   if (!NumROBEntries)

llvm/test/tools/llvm-mca/AMDGPU/gfx10-add-sequence.s

+# NOTE: Assertions have been autogenerated by utils/update_mca_test_checks.py
+# RUN: llvm-mca -mtriple=amdgcn -mcpu=gfx1010 --timeline --iterations=1 < %s | FileCheck %s
+
+v_add_f32 v0, v0, v0
+v_add_f32 v1, v1, v1
+v_add_f32 v2, v1, v0
+
+# CHECK:      Iterations:        1
+# CHECK-NEXT: Instructions:      3
+# CHECK-NEXT: Total Cycles:      13
+# CHECK-NEXT: Total uOps:        3
+
+# CHECK:      Dispatch Width:    1
+# CHECK-NEXT: uOps Per Cycle:    0.23
+# CHECK-NEXT: IPC:               0.23
+# CHECK-NEXT: Block RThroughput: 3.0
+
+# CHECK:      Instruction Info:
+# CHECK-NEXT: [1]: #uOps
+# CHECK-NEXT: [2]: Latency
+# CHECK-NEXT: [3]: RThroughput
+# CHECK-NEXT: [4]: MayLoad
+# CHECK-NEXT: [5]: MayStore
+# CHECK-NEXT: [6]: HasSideEffects (U)
+
+# CHECK:      [1]    [2]    [3]    [4]    [5]    [6]    Instructions:
+# CHECK-NEXT:  1      5     1.00                  U     v_add_f32_e32 v0, v0, v0
+# CHECK-NEXT:  1      5     1.00                  U     v_add_f32_e32 v1, v1, v1
+# CHECK-NEXT:  1      5     1.00                  U     v_add_f32_e32 v2, v1, v0
+
+# CHECK:      Resources:
+# CHECK-NEXT: [0]   - HWBranch
+# CHECK-NEXT: [1]   - HWExport
+# CHECK-NEXT: [2]   - HWLGKM
+# CHECK-NEXT: [3]   - HWRC
+# CHECK-NEXT: [4]   - HWSALU
+# CHECK-NEXT: [5]   - HWVALU
+# CHECK-NEXT: [6]   - HWVMEM
+
+# CHECK:      Resource pressure per iteration:
+# CHECK-NEXT: [0]    [1]    [2]    [3]    [4]    [5]    [6]
+# CHECK-NEXT:  -      -      -     3.00    -     3.00    -
+
+# CHECK:      Resource pressure by instruction:
+# CHECK-NEXT: [0]    [1]    [2]    [3]    [4]    [5]    [6]    Instructions:
+# CHECK-NEXT:  -      -      -     1.00    -     1.00    -     v_add_f32_e32 v0, v0, v0
+# CHECK-NEXT:  -      -      -     1.00    -     1.00    -     v_add_f32_e32 v1, v1, v1
+# CHECK-NEXT:  -      -      -     1.00    -     1.00    -     v_add_f32_e32 v2, v1, v0
+
+# CHECK:      Timeline view:
+# CHECK-NEXT:                     012
+# CHECK-NEXT: Index     0123456789
+
+# CHECK:      [0,0]     DeeeeER   . .   v_add_f32_e32 v0, v0, v0
+# CHECK-NEXT: [0,1]     .DeeeeER  . .   v_add_f32_e32 v1, v1, v1
+# CHECK-NEXT: [0,2]     .    .DeeeeER   v_add_f32_e32 v2, v1, v0
+
+# CHECK:      Average Wait times (based on the timeline view):
+# CHECK-NEXT: [0]: Executions
+# CHECK-NEXT: [1]: Average time spent waiting in a scheduler's queue
+# CHECK-NEXT: [2]: Average time spent waiting in a scheduler's queue while ready
+# CHECK-NEXT: [3]: Average time elapsed from WB until retire stage
+
+# CHECK:            [0]    [1]    [2]    [3]
+# CHECK-NEXT: 0.     1     0.0    0.0    0.0       v_add_f32_e32 v0, v0, v0
+# CHECK-NEXT: 1.     1     0.0    0.0    0.0       v_add_f32_e32 v1, v1, v1
+# CHECK-NEXT: 2.     1     0.0    0.0    0.0       v_add_f32_e32 v2, v1, v0
+# CHECK-NEXT:        1     0.0    0.0    0.0       <total>

llvm/test/tools/llvm-mca/AMDGPU/lit.local.cfg

+if not 'AMDGPU' in config.root.targets:
+    config.unsupported = True