[PATCH] D27243: Initial work on the XRay Graph tool.

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added a comment.

The formatting changes I suspect can be automated away by using clang-format, so we don't have to spend too many cycles just getting those things "right". :)



================
Comment at: tools/llvm-xray/xray-graph.cc:174-175
+  S.Pct99 = *(begin + Pct99Off);
+}
+void GraphRenderer::normaliseStatistics(double CycleFrequency) {
+  for (auto &V : Graph) {
----------------
Empty line between these two lines?


================
Comment at: tools/llvm-xray/xray-graph.cc:187-196
+  for (auto &V : VertexAttrs){
+    auto &S = V.second.S;
+
+  S.Min /= CycleFrequency;
+  S.Median /= CycleFrequency;
+  S.Max /= CycleFrequency;
+  S.Sum /= CycleFrequency;
----------------
The indentation here is a little weird. Fix?


================
Comment at: tools/llvm-xray/xray-graph.h:115-124
+  /// Calculates latency statistics for each edge and stores the data in the
+  /// Graph
+  void calculateEdgeStatistics(void);
+
+  ///Calculates latency statistics for each vertex and stores the data in the
+  /// Graph
+  void calculateVertexStatistics(void);
----------------
varno wrote:
> dberris wrote:
> > Can this not happen incrementally, as we're adding the records, or on demand when we export? i.e. why do they need to be public functions?
> Not really, all the records need to be added before we calculate the statistics. And I was wanting the statistics calculated to do graph transformations later (before output).
Sure, but why aren't these just implementation details of the exportGraphAsDOT(...) function?


================
Comment at: tools/llvm-xray/xray-graph.h:126-127
+private:
+  /// A private function to facilitate the output of edge labels;
+  void outputEdgeInfo(const TimeStat &S, StatType T, raw_ostream &OS) const;
+};
----------------
varno wrote:
> dberris wrote:
> > Does this need to be part of the class? Could this not be a function in the implementation?
> I suppose it could be a friend function, it uses private types (the TimeStat type).
It doesn't have to use that type, right? I suspect you could just make this a template in the unnamed namespace in the implementation. Or that type could just be public and you could just use it (i.e for example you might want to be able to provide an iterator to the graph later, so that other commands can leverage the graph too).


https://reviews.llvm.org/D27243