[PATCH] D66526: [utils] Add the llvm-locstats tool

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro marked 2 inline comments as done.
djtodoro added inline comments.


================
Comment at: llvm/trunk/test/tools/llvm-locstats/locstats.ll:1-2
+; RUN: llc %s -o %t0.o -filetype=obj \
+; RUN:   | %llvm-locstats %t0.o | FileCheck %s --check-prefix=LOCSTATS
+;
----------------
rupprecht wrote:
> The pipe should be deleted; `llc` is generating `%t0.o` as an intermediate object file and `llvm-locstats` is parsing that, not stdin. The stdout of llc is empty.
> 
> (If you want to support stdin, that should be a different test case)
Thanks! I have missed this..


================
Comment at: llvm/trunk/utils/llvm-locstats/llvm-locstats.py:107-109
+  # Get the directory of the LLVM tools.
+  llvm_dwarfdump_cmd = os.path.join(os.path.dirname(__file__), \
+                                    "llvm-dwarfdump")
----------------
rupprecht wrote:
> rupprecht wrote:
> > This assumes that `llvm-dwarfdump` exists in the same directory as `llvm-locstats`, which may not be true, e.g. for build systems that build dwardump and locstats in separate build trees. Can this be configurable? Possibly a regular `$PATH` lookup makes sense here (with cwd as a preferred location), although allowing a flag pointing to the location of dwarfdump would be the most flexible.
> (Note: feel free to reland this bit as-is, I can suggest this in the form of a patch once this landing has stuck).
Sure... A custom build system could make situation like that, I also thought about some improvements, but that can go as separate patches.

>(Note: feel free to reland this bit as-is, I can suggest this in the form of a patch once this landing has stuck).

Let's do that way. Please feel free to post such patch! :)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66526/new/

https://reviews.llvm.org/D66526