[PATCH] D137440: [RISCV][llvm-mca] Use LMUL Instruments to provide more accurate reports on RISCV

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

myhsu added a comment.

I only have minor comments. Otherwise LGTM. Cheers :-)

Also it will be great if you can mark inline comments as done upon addressing them, so that it's easier for both parties to navigate outstanding comments by simply clicking on "X comments" button at the top right corner. Another related tip: you can check those "done" boxes before updating a new diff. Phabricator will automatically submit them after uploading the diff.



================
Comment at: llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.cpp:98
+
+  for (auto I : IVec) {
+    // Unknown Instrument kind
----------------
this creates a copy for each element in `IVec` and brings some overhead due to extra bookkeeping upon copying a shared_ptr (and when `I` goes out of scope). I think you can `auto &I : IVec` (or even `const auto& I : IVec`) in this case.


================
Comment at: llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.cpp:105
+      // Not a RVV instr
+      if (RVV == nullptr) {
+        LLVM_DEBUG(
----------------



================
Comment at: llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.h:32
+  static const llvm::StringRef DESC_NAME;
+  static bool isDataValid(llvm::StringRef Data);
+
----------------
remove "llvm::". ditto for other occurrences in this patch


================
Comment at: llvm/tools/llvm-mca/CodeRegion.h:169
+
+  bool isRegionActive(llvm::StringRef Description) {
+    return ActiveRegions.find(Description) != ActiveRegions.end();
----------------
nit: const qualifier


================
Comment at: llvm/tools/llvm-mca/CodeRegion.h:174
+
+class AnalysisRegions : public CodeRegions {
+public:
----------------
nit: use struct instead and remove "public:". Ditto for InstrumentRegions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137440