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

Dean Michael Berris via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 6 19:57:08 PST 2016


dberris added a comment.

Some style comments.

As for tests I think this is an OK set for first tests, but it'd be good to look at error conditions and making sure we're handling those appropriately.



================
Comment at: tools/llvm-xray/xray-graph.cc:121
+      VertexAttrs[Record.FuncId].SymbolName = FuncIdHelper.SymbolOrNumber(Record.FuncId);
+    ThreadStack.push_back({Record.FuncId, Record.TSC});
+    break;
----------------
Can you use `.emplace_back(...)` instead here?


================
Comment at: tools/llvm-xray/xray-graph.cc:155-156
+  return true;
+}
+template <typename U, typename V>
+void GraphRenderer::getStats(U begin, V end, GraphRenderer::TimeStat &S){
----------------
Please add an empty line between the end of function definitions, and the start of the next function.


================
Comment at: tools/llvm-xray/xray-graph.h:27-28
+namespace llvm {
+namespace xray {
+/// A class encapsulating the logic related to analyzing XRay traces, producting
+/// Graphs from them and then exporting those graphs for review.
----------------
Please add an empty line between last non-comment and first comment lines.


================
Comment at: tools/llvm-xray/xray-graph.h:75
+  /// FIXME: Perhaps we can Build this into LatencyAccountant? or vise versa?
+  DenseMap<pid_t, std::vector<FunctionAttr>> PerThreadFunctionStack;
+
----------------
Can you use a `SmallVector<...>` instead of a `std::vector<...>`?


================
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);
----------------
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?


================
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;
+};
----------------
Does this need to be part of the class? Could this not be a function in the implementation?


https://reviews.llvm.org/D27243





More information about the llvm-commits mailing list