[PATCH] D68714: [MCA] Show aggregate over Average Wait times for the whole snippet (PR43219)
Andrea Di Biagio via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 9 12:44:28 PDT 2019
andreadb added inline comments.
================
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)
----------------
lebedev.ri wrote:
> andreadb wrote:
> > 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.
> To be honest i do not understand this comment.
> Is this better or worse?
> This does not help with coloring.
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68714/new/
https://reviews.llvm.org/D68714
More information about the llvm-commits
mailing list