[PATCH] D68714: [MCA] Show aggregate over Average Wait times for the whole snippet (PR43219)
Roman Lebedev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 9 11:26:44 PDT 2019
lebedev.ri 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)
----------------
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.
================
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,
----------------
andreadb wrote:
> 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.
Right.
Not by much though.
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