Implemented color coding and Vertex labels in XRay Graph

[varno]

Summary

A patch to enable the llvm-xray graph subcommand to color edges and
vertices based on statistics and to annotate vertices with statistics.

Depends on D27243

[varno]

• Fixing Test with changes added to handle color.

[dblaikie]

Needs test coverage for the color functionality, I think? (I don't see any colors being tested in the tests)

tools/llvm-xray/xray-graph.cc

264–270 ↗

(On Diff #82966)

Could use a member pointer & a helper function (possibly template) to maybe make these shorter/more reliable (by only having to mention the member name once - less chance of accidentally mismatching them).

310 ↗

(On Diff #82966)

Typo, 'function'

Also probably s/Normalise Statistics/normaliseStatistics/

Also, normalize rather than normalise (consistency with the rest of the codebase)

419–442 ↗

(On Diff #82966)

Could use another member pointer helper to handle this table, if you like.

[dberris]

tools/llvm-xray/xray-graph.cc

373–377 ↗

(On Diff #82966)

Probably missing a few punctuation marks. I think some structured fomulae might help with what exactly it's doing, like:

// Evaluates a polynomial given the provided range of coefficients in order.
//
//    p(x) = a[0]*x^0 + a[1]*x^1 + a[2]*x^2 + ... + a[n]*x^n
//
// We use Horner's Method for numerical stability for mapping to an 8-bit
// fixed point domain.

394–402 ↗

(On Diff #82966)

You probably want to document how you came up with these polynomials, even potentially citing some sources on how one might possibly start changing these.

405–407 ↗

(On Diff #82966)

Reading this, I think you're better off making polyEval take an llvm::ArrayRef instead (to make it easier to read).

444 ↗

(On Diff #82966)

Can you document why compare is returning the square root? You mentioned this offline in our discussion, but it would be good to document the reasons here.

[varno]

• Clang format

[varno]

Fixed and addressed comments.

[varno]

tools/llvm-xray/xray-graph.cc

264–270 ↗

(On Diff #82966)

Discussion resulted in this comment being seen as not easy to do due to specifics.

[dberris]

In general I think this could do with a bit more documentation, about the semantics of what we're trying to do (as opposed to how we're doing it).

tools/llvm-xray/xray-graph.cc

170 ↗

(On Diff #84218)

Empty line after this?

171 ↗

(On Diff #84218)

This is a template, no need to be static (it's implicitly inline).

175 ↗

(On Diff #84218)

Style guide says identifiers ought to have an uppercase start letter.

184–187 ↗

(On Diff #84218)

Can we describe this in less round-about way?

We can spend a few words in documentation to indicate what the side-effects are and some detail of what the algorithm being implemented is.

192–193 ↗

(On Diff #84218)

Was the intent to use llvm::Expected<...>? In which case I'd prefer you change this now.

[dberris]

Also, I think you'd want to rebase this given that llvm-xray graph has landed upstream.

[varno]

• Clang format
• Fixed tests after rebase
• Addressed review comments

[varno]

Also has been rebased to master.

[dberris]

tools/llvm-xray/xray-graph.cc

396 ↗

(On Diff #85324)

Empty line after this?

398 ↗

(On Diff #85324)

s/propper/proper/

409–410 ↗

(On Diff #85324)

Leave out the "I" and "Mathematica" parts -- explain the concepts involved, not the mechanics.

416–417 ↗

(On Diff #85324)

Instead of "subjectively provided a good result", say something like: "we used a 9-degree polynomial arbitrarily based on a fuzzy goodness of fit metric (using human judgment)".

430–431 ↗

(On Diff #85324)

Unnecessary now?

439–440 ↗

(On Diff #85324)

s/annother/another/

572 ↗

(On Diff #85324)

Do you need the equivalent of a "keep-going" flag here too (similar to what we have in llvm-xray account)?

[varno]

• Reply to comments

[dberris]

tools/llvm-xray/xray-graph.cc

580–586 ↗

(On Diff #85507)

This looks like it'd be better as:

if (auto E = GR.accountRecord(Record)) {
  ...
}

572 ↗

(On Diff #85324)

Okay, now I think you need to report errors encountered that we're ignoring in case we do want to keep going. Accumulating the individual errors into a long list seems unnecessary to me.

[varno]

• Respond to comments
• clang-format new

[varno]

• clang-format fix

[varno]

tools/llvm-xray/xray-graph.cc

580–586 ↗

(On Diff #85507)

This does not quite work here.

[dberris]

LGTM -- we can probably use some more user-facing documentation explaining a bit what this tool actually does (probably time to update docs/XRay.rst) but something to be done for later.

tools/llvm-xray/xray-graph.cc

581 ↗

(On Diff #85645)

In the future, comments like these really ought to expound more on what it's actually doing at a semantic level to help the reader of the code. A more informative comment as to "why" we're doing something might make more sense to the reader as that's not immediately obvious. Something like:

// Populate the graph with function call records that we
// encounter from the XRay trace. Any errors we encounter
// will be printed through standard error.

[dblaikie]

llvm/trunk/tools/llvm-xray/xray-graph.cc

212–213

Should probably be using llvm::errc - see the Support/errc.h - it defines portable constants we know work across all the compilers LLVM supports, etc.

401–403

Usually don't bother with {} on single line scopes.

586–592

Should accountRecord do this work/include this information in its Error (rather than callers doing so)?

llvm/trunk/tools/llvm-xray/xray-graph.h

135

Const value return type? I'm assuming this should be a const ref return type? (we should really have a clang-tidy for that if we don't already, maybe even a straight-up compiler warning)