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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: test/tools/llvm-xray/X86/graph-simple-case.yaml:34-35
+#EMPTY:   digraph xray {
+#EMPTY:   F0 -> F1 [label="1"];
+#EMPTY:   F1 [label="@(1)"];
+#EMPTY:   }
----------------
Did you need to write "DAG" here somewhere too?


================
Comment at: tools/llvm-xray/CMakeLists.txt:15
+  xray-graph.cc
   xray-log-reader.cc
   xray-registry.cc)
----------------
Rebasing this to tip of trunk now that 'llvm-xray account' has landed might mean this dependency goes away now.


================
Comment at: tools/llvm-xray/xray-graph.cc:211-212
+  }
+  std::vector<uint64_t> TempTimings;
+  TempTimings.reserve(MaxCount);
+  for (auto &V : IncommingEdges) {
----------------
varno wrote:
> dblaikie wrote:
> > Is this optimization worthwhile? Or could we put this as a local variable in the outer loop below - no need to clear it, etc.
> I think it is worth while, however I have no testing results. At this time, as I am working on a patch which changes how this code works completely due to a new data structure, I don't know.
I'd urge you to not change this patch, but instead stack one on top of it instead for any further changes you'd need to do. I'd much rather do that review of the refactoring separately than doing this review over with new data structures.


================
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));
----------------
dblaikie wrote:
> 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)
I think we at least should print something to the effect of "well, we can't symbolise properly". I'm happy with making this an explicit error, to not give users potentially misleading results.


================
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));
----------------
dblaikie wrote:
> Move this to where it's used (then you could even use 'const auto *' if you like
Given the changes that have landed now, there's a better way of doing this (if we look at what's happening in `llvm-xray account` at least).


https://reviews.llvm.org/D27243