[PATCH] D89892: [AsmPrinter] Add per BB instruction mix remark.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1125
     // Get MachineLoopInfo or compute it on the fly if it's unavailable
     MLI = getAnalysisIfAvailable<MachineLoopInfo>();
     if (!MLI) {
----------------
thegameg wrote:
> Looks like we can have loop info around here too, maybe a nice future extension would be to emit a remark with the instruction mix per loop too.
Sounds like a useful extension in the future indeed!


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1208
         emitInstruction(&MI);
+        if (CanDoExtraAnalysis) {
+          auto I = OpcodeCounts.insert({MI.getOpcode(), 0u});
----------------
paquette wrote:
> Will this work with inline assembly? Looks like inline assembly is handled earlier in the switch.
> 
> (Probably fine to support it later anyway.)
Hmm, I am not sure we can really do much about inline assembly in general. Isn't that just a opaque string passed through?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1268
+      for (auto &KV : OpcodeCounts) {
+        auto Name = (Twine("INST_") + TII->getName(KV.first)).str();
+        OpcodeCountsVec.emplace_back(Name, KV.second);
----------------
paquette wrote:
> Is there any reason that you're prefixing these with `INST_`? Are you planning on adding other ways to categorise these?
The prefix is intended to make it easier for tool to post-process the information to disambiguate the counts from other info (like the BB). But at the moment it is not really necessary and mostly for convenience. For now I am not planning on adding other ways to categorize the instruction counts. That seems tricky to do here in a target-independent way unfortunately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89892