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

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added inline comments.


================
Comment at: test/tools/llvm-xray/X86/graph-deduce-tail-call.yaml:8-17
+#RUN: llvm-xray graph %s -i=yaml -o - -m %S/Inputs/simple-instrmap.yaml -t yaml -d -e med \
+#RUN:    | FileCheck %s -check-prefix=TIME
+#RUN: llvm-xray graph %s -i=yaml -o - -m %S/Inputs/simple-instrmap.yaml -t yaml -d -e 90p \
+#RUN:    | FileCheck %s -check-prefix=TIME
+#RUN: llvm-xray graph %s -i=yaml -o - -m %S/Inputs/simple-instrmap.yaml -t yaml -d -e 99p \
+#RUN:    | FileCheck %s -check-prefix=TIME
+#RUN: llvm-xray graph %s -i=yaml -o - -m %S/Inputs/simple-instrmap.yaml -t yaml -d -e max \
----------------
This seems to be passing a variety of values for '-e' but tests that the behavior is the same in all of them. I'm assuming the flag does something - do you have tests that confirm that it does the right thing? (somewhat similar for the top two test cases too)


================
Comment at: test/tools/llvm-xray/X86/graph-deduce-tail-call.yaml:39-40
+#
+# Note that by default, tail/sibling call deduction is disabled, and is enabled
+# with a flag "-d" or "-deduce-sibling-calls".
+#
----------------
Is deduce-sibling-calls tested?


================
Comment at: tools/llvm-xray/xray-graph.cc:211-212
+  }
+  std::vector<uint64_t> TempTimings;
+  TempTimings.reserve(MaxCount);
+  for (auto &V : IncommingEdges) {
----------------
Is this optimization worthwhile? Or could we put this as a local variable in the outer loop below - no need to clear it, etc.


================
Comment at: tools/llvm-xray/xray-graph.cc:222
+// Here is an alternative body of this function
+#if 0
+  DenseMap<int32_t, Smallvector<uint64_t, 5>> VertexData;
----------------
Remove dead code


================
Comment at: tools/llvm-xray/xray-graph.cc:287-288
+
+// Does what the name suggests, it creates a DOT file from the Graph embedded in
+// the GraphRenderer object. It does this in the expected way by itterating
+// through all edges then vertices and then outputting them and their
----------------
Probably skip the extra language like "does what the name suggests" and "does this in the expected way".

If it's pretty obvious/self explanatory, then a brief comment is OK.


================
Comment at: tools/llvm-xray/xray-graph.cc:340-341
+          std::move(Err), [&](std::unique_ptr<StringError> E) -> Error {
+            if (E->convertToErrorCode() == std::errc::no_such_file_or_directory)
+              return Error::success();
+            return Error(std::move(E));
----------------
Should this be silently handled? If a user specifies a file, seems like we should error if it's not there

(@dberris - goes for existing tools too, I imagine, guess I didn't notice this in the others)


================
Comment at: tools/llvm-xray/xray-graph.cc:358
+  xray::GraphRenderer GR(FuncIdHelper, GraphDeduceSiblingCalls);
+  const llvm::xray::XRayFileHeader *Header = nullptr;
+  LogReader Reader(GraphInput, Err, true, getSupportedLoader(GraphInputFormat));
----------------
Move this to where it's used (then you could even use 'const auto *' if you like


https://reviews.llvm.org/D27243