Page MenuHomePhabricator

[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.
ClosedPublic

Authored by holland11 on Jun 23 2021, 1:46 PM.

Details

Summary

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.

Diff Detail

Event Timeline

holland11 created this revision.Jun 23 2021, 1:46 PM
holland11 requested review of this revision.Jun 23 2021, 1:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2021, 1:46 PM
This revision is now accepted and ready to land.Jun 23 2021, 2:30 PM