[PATCH] D28225: Implemented color coding and Vertex labels in XRay Graph
Dean Michael Berris via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Jan 22 22:38:10 PST 2017
dberris requested changes to this revision.
dberris added inline comments.
This revision now requires changes to proceed.
================
Comment at: tools/llvm-xray/xray-graph.cc:396
+ return B;
+}
+// Takes a double precision number, clips it between 0 and 1 and then converts
----------------
Empty line after this?
================
Comment at: tools/llvm-xray/xray-graph.cc:398
+// Takes a double precision number, clips it between 0 and 1 and then converts
+// that to an integer between 0x00 and 0xFF with propper rounding.
+static uint8_t uintIntervalTo8bitChar(double B) {
----------------
s/propper/proper/
================
Comment at: tools/llvm-xray/xray-graph.cc:409-410
+//
+// In order to calculate these polynomials, I used Mathematica to do the
+// following:
+// 1. Convert the OrRed9 color scheme from http://colorbrewer2.org/ from sRGB
----------------
Leave out the "I" and "Mathematica" parts -- explain the concepts involved, not the mechanics.
================
Comment at: tools/llvm-xray/xray-graph.cc:416-417
+// 3. Sample this interpolation at 100 points and convert to sRGB.
+// 4. Calculate a polynomial fit for these 100 points for each of R G and B.
+// using a degree 9 polynominal subjectively provided a good result.
+// 5. Extract these polynomial coefficients from matlab as a set of constants.
----------------
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)".
================
Comment at: tools/llvm-xray/xray-graph.cc:430-431
+ 0.732108, 0.913916};
+ using std::begin;
+ using std::end;
+ uint8_t r = uintIntervalTo8bitChar(polyEval(RedPoly, point));
----------------
Unnecessary now?
================
Comment at: tools/llvm-xray/xray-graph.cc:439-440
+
+// Returns the quotient between the property T of this and annother TimeStat as
+// a double
+double GraphRenderer::TimeStat::compare(StatType T, const TimeStat &O) const {
----------------
s/annother/another/
================
Comment at: tools/llvm-xray/xray-graph.cc:572
for (const auto &Record : Trace) {
// Generate graph, FIXME: better error recovery.
+ auto ER2 = GR.accountRecord(Record);
----------------
Do you need the equivalent of a "keep-going" flag here too (similar to what we have in `llvm-xray account`)?
https://reviews.llvm.org/D28225
More information about the llvm-commits
mailing list