[PATCH] D27243: Initial work on the XRay Graph tool.
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 9 13:14:46 PST 2017
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
More information about the llvm-commits
mailing list