[PATCH] D118181: [InstrProf][Correlate] Verify debug info with llvm-profdata show

Petr Hosek via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jan 26 00:16:44 PST 2022


phosek added inline comments.


================
Comment at: compiler-rt/test/profile/Linux/instrprof-show-debug-info-correlation.c:2
+// RUN: %clang_pgogen -o %t -g -mllvm --debug-info-correlate -mllvm --disable-vp=true %s
+// RUN: llvm-profdata show --debug-info-correlation --detailed-summary --show-prof-sym-list %t | FileCheck %s
+
----------------
kyulee wrote:
> It appears there are many similar names, which is confusing to remember.
> The compilation uses `-debug-info-correlate`.
> For `llvm-profdata merge`, use `-debug-info=<debug file>`.
> Now, `llvm-profdata show` use `-debug-info-correlation`. This usage actually specifies `<debug file>` in the place of `<profile file>` which is a required argument to `llvm-profdata show`. 
> One thing I can think about is to use `llvm-profdata show -debug-info=<debug file>` while allowing to omit the required `<profile file>`, which becomes optional. But this doesn't look ideal, either. 
> I don't have a strong suggestion, but I wish some more consistent naming. 
I agree and in particular I'd prefer the last suggestion, that is making `<profile file>` optional when `-debug-info=<debug file>` is specified. I think that changing the meaning of the positional argument (that is either a profile or a binary) depending on other arguments is more confusing.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:2581
+      "debug-info-correlation", cl::init(false),
+      cl::desc("Read and exctract profile metadata from debug info and show "
+               "the functions it found."));
----------------
Nit: spelling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118181



More information about the llvm-commits mailing list