[PATCH] D150459: [llvm-mca] Print InstructionInfoView using Instrument information.

[Min-Yih Hsu via Phabricator via llvm-commits]

myhsu added inline comments.


================
Comment at: llvm/tools/llvm-mca/Views/InstructionInfoView.cpp:123
+    unsigned SchedClassID =
+        IM.getSchedClassID(MCII, Inst, InstToInstruments.find(&Inst)->second);
     unsigned CPUID = SM.getProcessorID();
----------------
I think you need to check if `&Inst` exists in `InstToInstruments` here (If it's unlikely to happen then maybe put it as an assertion just in case)


================
Comment at: llvm/tools/llvm-mca/Views/InstructionInfoView.h:62
+  const InstrumentManager &IM;
+  const std::map<const MCInst *, const SmallVector<mca::SharedInstrument>>
+      &InstToInstruments;
----------------
I think it'll be better to use DenseMap here since the key is a pointer.


================
Comment at: llvm/tools/llvm-mca/Views/InstructionInfoView.h:84
+      bool ShouldPrintBarriers, const InstrumentManager &IM,
+      const std::map<const MCInst *, const SmallVector<mca::SharedInstrument>>
+          &InstToInstruments)
----------------
nit: could you use a type alias for `InstToInstruments`'s type? it'll be easier to read


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150459