[PATCH] D104675: [MCA] [In-order pipeline] Fix for 0 latency instruction causing assertion to fail.

[Patrick Holland via Phabricator via llvm-commits]

holland11 created this revision.
holland11 added reviewers: andreadb, qcolombet.
Herald added subscribers: gbedwell, hiraditya, tpr.
holland11 requested review of this revision.
Herald added a project: LLVM.

Within the in-order pipeline, when an instruction gets issued and dispatched, it gets placed in the `InOrderIssueStage::IssuedInst` list. Then, at the beginning of every cycle, this list is iterated over and each instruction has their `CyclesLeft` attribute decremented. If any instructions have their `CyclesLeft` attribute become 0, then functions are invoked to handle everything associated with an instruction finishing.

At this time, an instruction with 0 latency is handled the same as any other instruction. Once it's dispatched, it gets placed within the `IssuedInst` list. This is a problem because that list doesn't get iterated over until the following cycle so the instruction will be retired 1 cycle later than it should. Before that even happens though, we'll fail an `assert` and the program will terminate.

My tablegen investigative skills aren't very strong and I wasn't able to find an existing 0 latency instruction within any of the in-order upstream targets, but if you want to test it for yourself, you can follow these steps:

Within `llvm/lib/Target/AMDGPU/SISchedule.td`, change (around line 252)

  def : HWWriteRes<WriteIntMul,        [HWVALU, HWRC],   8>;

To

  def : HWWriteRes<WriteIntMul,        [],   0>;

Rebuild `llvm-mca` then run `llvm-mca` with the following parameters

  -mtriple=amdgcn -mcpu=gfx1010

On the following source input

  v_mul_lo_u32 v5, v1, v4

You should then get this error:

  Assertion failed: (isExecuting() && "Instruction not in-flight?"), function cycleEvent, file /llvm/lib/MCA/Instruction.cpp, line 235.

I believe that the in-order pipeline was intended to be able to handle 0 latency instructions, but they're pretty uncommon so it was probably never tested. If we look at `Instruction::execute()`

  void Instruction::execute(unsigned IID) {
    assert(Stage == IS_READY);
    Stage = IS_EXECUTING;
  
    // Set the cycles left before the write-back stage.
    CyclesLeft = getLatency();
  
    for (WriteState &WS : getDefs())
      WS.onInstructionIssued(IID);
  
    // Transition to the "executed" stage if this is a zero-latency instruction.
    if (!CyclesLeft)
      Stage = IS_EXECUTED;
  }

We can see a comment near the bottom that demonstrates this. This function is invoked from within `InOrderIssueStage::tryIssue()` and the code that I added comes a few lines after `Instruction::execute()` is run. Since `Instruction::execute()` will set 0 latency instructions to `IS_EXECUTED`, my new block checks for this state and invokes the appropriate 'retirement' functions (and then returns so that we don't end up adding the instruction to the `IssuedInst` list).

After making this fix, I noticed that the `TimelineView` was not working properly if the first instruction in the source code was a 0 latency instruction. That is the motivation for the change I made to `TimelineView.cpp`. I'm not quite sure why that if-statement -> early exit block is in that loop. I tried using `git blame` and `git log` to find any clues, but couldn't figure it out. I then asked Andrea and he wasn't sure either so I think it should be safe to remove it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104675

Files:
  llvm/lib/MCA/Stages/InOrderIssueStage.cpp
  llvm/tools/llvm-mca/Views/TimelineView.cpp


Index: llvm/tools/llvm-mca/Views/TimelineView.cpp
===================================================================
--- llvm/tools/llvm-mca/Views/TimelineView.cpp
+++ llvm/tools/llvm-mca/Views/TimelineView.cpp
@@ -288,8 +288,6 @@
   for (unsigned Iteration = 0; Iteration < Iterations; ++Iteration) {
     for (const MCInst &Inst : Source) {
       const TimelineViewEntry &Entry = Timeline[IID];
-      if (Entry.CycleRetired == 0)
-        return;
 
       unsigned SourceIndex = IID % Source.size();
       printTimelineViewEntry(FOS, Entry, Iteration, SourceIndex);
Index: llvm/lib/MCA/Stages/InOrderIssueStage.cpp
===================================================================
--- llvm/lib/MCA/Stages/InOrderIssueStage.cpp
+++ llvm/lib/MCA/Stages/InOrderIssueStage.cpp
@@ -241,6 +241,18 @@
     Bandwidth = Desc.EndGroup ? 0 : Bandwidth - NumMicroOps;
   }
 
+  // If the instruction has a latency of 0, we need to handle
+  // the execution and retirement now.
+  if (IS.isExecuted()) {
+    PRF.onInstructionExecuted(&IS);
+    notifyEvent<HWInstructionEvent>(
+        HWInstructionEvent(HWInstructionEvent::Executed, IR));
+    LLVM_DEBUG(dbgs() << "[E] Instruction #" << IR << " is executed\n");
+
+    retireInstruction(IR);
+    return llvm::ErrorSuccess();
+  }
+
   IssuedInst.push_back(IR);
 
   if (!IR.getInstruction()->getDesc().RetireOOO)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D104675.353517.patch
Type: text/x-patch
Size: 1365 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210622/d80248cb/attachment.bin>