[PATCH] D154526: [llvm-mca][RISCV] vsetivli and vsetvli act as instruments

[Michael Maitland via Phabricator via llvm-commits]

michaelmaitland added inline comments.


================
Comment at: llvm/include/llvm/MCA/CustomBehaviour.h:164
+  /// is allocated by this function.
+  virtual SmallVector<UniqueInstrument> createInstruments(const MCInst &Inst);
+
----------------
myhsu wrote:
> I feel like the comment here (and maybe that of `createInstrument(StringRef,StringRef)`) can hint that there are two ways to create Instruments -- by string description + data or from MCInst
I've added this hint on both functions.


================
Comment at: llvm/tools/llvm-mca/CodeRegionGenerator.h:121-127
+  InstrumentRegions &Regions;
+  InstrumentManager &IM;
+
+public:
+  InstrumentMCStreamer(MCContext &Context, mca::InstrumentRegions &R,
+                       InstrumentManager &IM)
+      : MCStreamerWrapper(Context, R), Regions(R), IM(IM) {}
----------------
andreadb wrote:
> Have you considered different approaches? Regions are already available from the base class. We don't need two references to the same data.
I've updated this to avoid duplicate reference.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154526