[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) {

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 {

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`)?


More information about the llvm-commits mailing list