[PATCH] D104815: [MCA] [TimelineView] Fixing a bug (that I caused) in TimelineView.cpp that was causing instructions that execute after timeline-max-cycles to still be displayed.

[Patrick Holland via Phabricator via llvm-commits]

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

Within this diff https://reviews.llvm.org/D104675, I tried to fix a bug that was causing 0 latency instructions that were being retired within cycle 0 to break the timeline view. The way I fixed it was insufficient, because I didn't understand the reason for the if statement in `TimelineView::printTimeline()`. I now understand its purpose so I've modified my 'fix' to still behave as intended, but to also still fix the 0 latency issue.

To be more clear about what's happening, let's look at `TimelineView::onEvent()`:

  void TimelineView::onEvent(const HWInstructionEvent &Event) {
    const unsigned Index = Event.IR.getSourceIndex();
    if (Index >= Timeline.size())
      return;
  
    switch (Event.Type) {
    case HWInstructionEvent::Retired: {
      TimelineViewEntry &TVEntry = Timeline[Index];
      if (CurrentCycle < MaxCycle)
        TVEntry.CycleRetired = CurrentCycle;
  
      // Update the WaitTime entry which corresponds to this Index.
      assert(TVEntry.CycleDispatched >= 0 && "Invalid TVEntry found!");
      unsigned CycleDispatched = static_cast<unsigned>(TVEntry.CycleDispatched);
      WaitTimeEntry &WTEntry = WaitTime[Index % getSource().size()];
      WTEntry.CyclesSpentInSchedulerQueue +=
          TVEntry.CycleIssued - CycleDispatched;
      assert(CycleDispatched <= TVEntry.CycleReady &&
             "Instruction cannot be ready if it hasn't been dispatched yet!");
      WTEntry.CyclesSpentInSQWhileReady +=
          TVEntry.CycleIssued - TVEntry.CycleReady;
      if (CurrentCycle > TVEntry.CycleExecuted) {
        WTEntry.CyclesSpentAfterWBAndBeforeRetire +=
            (CurrentCycle - 1) - TVEntry.CycleExecuted;
      }
      break;
    }
    case HWInstructionEvent::Ready:
      Timeline[Index].CycleReady = CurrentCycle;
      break;
    case HWInstructionEvent::Issued:
      Timeline[Index].CycleIssued = CurrentCycle;
      break;
    case HWInstructionEvent::Executed:
      Timeline[Index].CycleExecuted = CurrentCycle;
      break;
    case HWInstructionEvent::Dispatched:
      // There may be multiple dispatch events. Microcoded instructions that are
      // expanded into multiple uOps may require multiple dispatch cycles. Here,
      // we want to capture the first dispatch cycle.
      if (Timeline[Index].CycleDispatched == -1)
        Timeline[Index].CycleDispatched = static_cast<int>(CurrentCycle);
      break;
    default:
      return;
    }
    if (CurrentCycle < MaxCycle)
      LastCycle = std::max(LastCycle, CurrentCycle);
  }

We can see that when an instruction is Retired, if `CurrentCycle` is not less than `MaxCycle`, `CycleRetired` stays 0. However, when an instruction is Executed, we set `CycleExecuted` regardless.

So the new change that I am making now will check for `CycleRetired == 0` to determine whether this instruction was Retired within the timeline limits, but it will also check `CycleExecuted` to make sure we aren't looking at a 0 cycle instruction that has a legitimate `CycleRetired` set to 0.

It's possible that during these few days where the bug was present, some new llvm-mca related tests were made (that could now fail). But I ran both `ninja check all` and `ninja check llvm-mca` and there were no failures so we should be fine.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104815

Files:
  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,6 +288,15 @@
   for (unsigned Iteration = 0; Iteration < Iterations; ++Iteration) {
     for (const MCInst &Inst : Source) {
       const TimelineViewEntry &Entry = Timeline[IID];
+      // When an instruction is retired after timeline-max-cycles,
+      // its CycleRetired is left at 0. However, it's possible for
+      // a 0 latency instruction to be retired during cycle 0 and we
+      // don't want to early exit in that case. The CycleExecuted
+      // attribute is set correctly whether or not it is greater
+      // than timeline-max-cycles so we can use that to ensure
+      // we don't early exit because of a 0 latency instruction.
+      if (Entry.CycleRetired == 0 && Entry.CycleExecuted != 0)
+        return;
 
       unsigned SourceIndex = IID % Source.size();
       printTimelineViewEntry(FOS, Entry, Iteration, SourceIndex);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D104815.354060.patch
Type: text/x-patch
Size: 1070 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210623/f7fc7b0c/attachment.bin>