[PATCH] D28225: Implemented color coding and Vertex labels in XRay Graph

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: tools/llvm-xray/xray-graph.cc:373-377
+// Evaluates a polynomial p(x) = a_0 + a_1*x^1 + ... a_n*x^n at x_0 using
+// Horner's Method, and then returns the value as an 8-bit fixed point
+// representation
+// The Polynomial coefficients are parsed in such that begin contains a_n
+// and following it are a_(n-1) ... in order.
----------------
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.
```


================
Comment at: tools/llvm-xray/xray-graph.cc:394-402
+  const static double RedPoly[]   = {-38.4295, 239.239, -600.108, 790.544,
+                                     -591.26, 251.304, -58.0983, 6.62999,
+                                     -0.325899, 1.00173};
+  const static double GreenPoly[] = {-603.634, 2338.15, -3606.74, 2786.16,
+                                     -1085.19, 165.15, 11.2584, -6.11338,
+                                     -0.0091078, 0.965469};
+  const static double BluePoly[]  = {-325.686, 947.415, -699.079, -513.75,
----------------
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.


================
Comment at: tools/llvm-xray/xray-graph.cc:405-407
+  uint8_t r = polyEval(begin(RedPoly),end(RedPoly), point);
+  uint8_t g = polyEval(begin(GreenPoly),end(GreenPoly),point);
+  uint8_t b = polyEval(begin(BluePoly),end(BluePoly),point);
----------------
Reading this, I think you're better off making `polyEval` take an `llvm::ArrayRef` instead (to make it easier to read).


================
Comment at: tools/llvm-xray/xray-graph.cc:444
+  }
+  return std::sqrt(retval);
 }
----------------
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.


https://reviews.llvm.org/D28225