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

[Alexis Shaw via Phabricator via llvm-commits]

varno marked 4 inline comments as done.
varno 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 \
----------------
dblaikie wrote:
> 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)
I need to add additional test cases for these,  however the test case size for -e 99p must be quite long in order for it to work. Will add in next revision of patch.


================
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".
+#
----------------
dblaikie wrote:
> Is deduce-sibling-calls tested?
Yes. Otherwise the graph would only have two nodes with timing information


================
Comment at: tools/llvm-xray/xray-graph.cc:211-212
+  }
+  std::vector<uint64_t> TempTimings;
+  TempTimings.reserve(MaxCount);
+  for (auto &V : IncommingEdges) {
----------------
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.


https://reviews.llvm.org/D27243