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

[David Blaikie via llvm-commits]

On Mon, Jan 9, 2017 at 2:43 PM Alexis Shaw via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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
>

Oh, I see - I went looking for the full flag name and saw no tests passing
it. I now reread the comment and see the test case is passing the short
name.


>
>
> ================
> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170109/3fb70c1d/attachment.html>