Page MenuHomePhabricator

[MCA] Show aggregate over Average Wait times for the whole snippet (PR43219)
ClosedPublic

Authored by lebedev.ri on Wed, Oct 9, 9:40 AM.

Details

Summary

As disscused in https://bugs.llvm.org/show_bug.cgi?id=43219,
i believe it may be somewhat useful to show some aggregates
over all the sea of statistics provided.

Example:

Average Wait times (based on the timeline view):
[0]: Executions
[1]: Average time spent waiting in a scheduler's queue
[2]: Average time spent waiting in a scheduler's queue while ready
[3]: Average time elapsed from WB until retire stage

      [0]    [1]    [2]    [3]
0.     3     1.0    1.0    4.7       vmulps     %xmm0, %xmm1, %xmm2
1.     3     2.7    0.0    2.3       vhaddps    %xmm2, %xmm2, %xmm3
2.     3     6.0    0.0    0.0       vhaddps    %xmm3, %xmm3, %xmm4
       3     3.2    0.3    2.3       <total>

I.e. we average the averages.

Diff Detail

Event Timeline

lebedev.ri created this revision.Wed, Oct 9, 9:40 AM

Thanks Roman.

It is a shame that so many tests are affected by this change. But that is obviously not your fault.
See my comments below.

llvm/tools/llvm-mca/Views/TimelineView.cpp
132–142

You can still use the old signature for this method (see the explanation below):

We know that we are printing the special <total> entry if SourceIndex == Source.size().

You can use that knowledge in two places:

  1. You can automatically infer flag ShouldNumber.
bool ShouldNumber = SourceIndex != Source.size();
if (ShouldNumber)
  OS << SourceIndex << '.';
  1. Before printing the average times, you can check if numbers are fore he special <total> entry and modify the value of Executions with Timeline.size() / Source.size().

You can do it where you currently added the FIXME comment.

if (!ShouldNumber) {
  // override Executions for the purpose of changing colors.
  Executions = Timeline.size() / Source.size();
}

Basically you don't need CumulativeExecutions as it can be inferred from the context. That should also fix the issue with the coloring of the output.

205–216

We should not print the special <total> entry if Source.size() == 1.

If the input assembly only contains a single instruction, then we know that the <total> entry is redundant.

It should also (hopefully) simplify the diff a bit.

lebedev.ri marked 3 inline comments as done.

Attempt to address nits.

llvm/tools/llvm-mca/Views/TimelineView.cpp
132–142

To be honest i do not understand this comment.
Is this better or worse?
This does not help with coloring.

205–216

Right.
Not by much though.

andreadb added inline comments.Wed, Oct 9, 12:43 PM
llvm/tools/llvm-mca/Views/TimelineView.cpp
132–142

Okay. I see the problem now.

At line 155 we have:

int BufferSize = UsedBuffer[SourceIndex].second;

However, SourceIndex is not a valid index if method printWaitTimeEntry() is called to print the <total>. The motivation is that SourceIndex is set to Source.size() for entry <total>, and there are only Source.size() elements in vector UsedBuffer. So that access is invalid if we are printing the <total>.

There is not an easy fix for it.
I suggest for now that we avoid to change the colors if we know that we are printing the <total> entry.

lebedev.ri marked 2 inline comments as done.

Fix coloring by fixing out-of-bounds read.
Thanks to @andreadb to noticing! :)

lebedev.ri edited the summary of this revision. (Show Details)Wed, Oct 9, 12:54 PM
lebedev.ri added inline comments.
llvm/tools/llvm-mca/Views/TimelineView.cpp
132–142

Oh, that would explain it..
I should have seen it :(

andreadb added inline comments.Thu, Oct 10, 4:10 AM
llvm/tools/llvm-mca/Views/TimelineView.cpp
157–158

if you use std::numeric_limits<int>::max() then you introduce an unsigned wrap in function chooseColor (lines 111 and 113) when CumulativeExecutions is bigger than 2.

Two options:

  1. you either avoid calling tryChangeColor if you are printing the totals, or
  2. you modify function chooseColor so that raw_ostream::SAVEDCOLOR is returned when BufferSize is 0. This requires that BufferSize is initialized to 0 (here at line 154) when printing the totals.
lebedev.ri marked 2 inline comments as done.

Avoid coloring for "total" line.

llvm/tools/llvm-mca/Views/TimelineView.cpp
157–158

Let's just not change color then.
This is starting to get somewhat ugly, which is what i was initially afraid of though.

This revision is now accepted and ready to land.Thu, Oct 10, 7:14 AM

LGTM

Alright, thank you very much for the review!

This revision was automatically updated to reflect the committed changes.