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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

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.



================
Comment at: llvm/tools/llvm-mca/Views/TimelineView.cpp:132-140
 void TimelineView::printWaitTimeEntry(formatted_raw_ostream &OS,
                                       const WaitTimeEntry &Entry,
                                       unsigned SourceIndex,
-                                      unsigned Executions) const {
-  OS << SourceIndex << '.';
+                                      unsigned CumulativeExecutions,
+                                      unsigned Executions,
+                                      bool ShouldNumber) const {
+  if (ShouldNumber)
----------------
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 << '.';
```

2) 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.


================
Comment at: llvm/tools/llvm-mca/Views/TimelineView.cpp:204-215
+
+  WaitTimeEntry TotalWaitTime = std::accumulate(
+      WaitTime.begin(), WaitTime.end(), WaitTimeEntry{0, 0, 0},
+      [](const WaitTimeEntry &A, const WaitTimeEntry &B) {
+        return WaitTimeEntry{
+            A.CyclesSpentInSchedulerQueue + B.CyclesSpentInSchedulerQueue,
+            A.CyclesSpentInSQWhileReady + B.CyclesSpentInSQWhileReady,
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68714/new/

https://reviews.llvm.org/D68714