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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added a comment.

Did you run this through clang-format in LLVM mode?



================
Comment at: tools/llvm-xray/xray-graph.cc:229-230
+  }
+}
+namespace {
+void outputEdgeInfo(const GraphRenderer::TimeStat &S, GraphRenderer::StatType T,
----------------
Missing empty line between these two.


================
Comment at: tools/llvm-xray/xray-graph.cc:344-347
+  GR.calculateEdgeStatistics();
+  GR.calculateVertexStatistics();
+  if (Header->CycleFrequency)
+    GR.normaliseStatistics(Header->CycleFrequency);
----------------
I'm looking at these lines and thinking it seems they're unnecessarily exposing implementation details -- really these things are something the graph renderer can decide itself if it took the file header as an argument to the constructor. And these really ought to just happen when we're exporting the data as DOT and passing arguments to the kind of data we want to see.


================
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:
> > 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?
> I suppose that right now they could be, but I have plans that require them to be run before exportGraphAsDOT
I suspect we can break these out later if we really need to. For now, they're a distraction and from an API design perspective, really brittle -- if someone wants to use this class they have to know to call these functions before exporting. If this doesn't happen while we're accounting the records, then this means the state of the graph is not easily determined.

If for example an external algorithm requires the graph, then we ought to be able to access the graph itself -- and maybe the accounting ought to happen externally instead, as an algorithm that passes through the graph we've built or as an extension to the graph traversal algorithm being employed, exposed as a method to this class. 


https://reviews.llvm.org/D27243