[PATCH] D125008: [llvm-objdump] Print Mnemonic Histogram

[David Spickett via Phabricator via llvm-commits]

DavidSpickett added a comment.

I don't know what bar there is to add stuff like this but I assume it's quite low given that it's self contained and not trying to match any equivalent GNU options. I can see the use for comparing files certainly.



================
Comment at: llvm/test/tools/llvm-objdump/ARM/mnemonic-hist1.test:17
+#CHECK: mul: 1 (25%)
+#CHECK: sub: 1 (25%)
----------------
If you're going to pad the names to make them all line up then you should test something that isn't 3 characters. Also, and this makes the test bigger but check that the number padding works. Maybe just make one appear 10 times, unless you can figure out a macro to emit 100 of something.

Also do you expect an ordering between the 2 instructions that both have 1? As in should it be alphabetical between those 2 or is it going to depend on order of appearance in the object file.

Edit: Putting them in a std::map sorts them so you would always see mul then sub even if sub appears first in the object, correct?

Can you comment in the test file what things it's looking for.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1491
+            int Bytes;
+            std::tie(Mnemonic, Bytes) = IP->getMnemonic(&Inst);
+            Hist[Mnemonic]++;
----------------
std::ignore here? Or std::get<0>(IP->getMnemonic(&Inst)) directly.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2171
+
+  std::sort(List.begin(), List.end(), [] (std::pair<std::string, unsigned> &L,
+                                          std::pair<std::string, unsigned> &R) {
----------------
Probably makes no difference but why not `const std::pair<std::string, unsigned> &`.


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

https://reviews.llvm.org/D125008