[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 reviewer: myhsu.
myhsu added a comment.

I read your patch descriptions with great interest and kudos on listing all cases. I generally agree with your instrument comment / analysis region. I think it's the most practical solution we have for now and also opens a door for creating other kinds of MCA extensions in the future.

On the technical side I dropped some drive-by comments, I'll have a deeper look later. As Andrea mentioned, please add some tests first. I think it's not hard in your case as you already had plenty of examples.



================
Comment at: llvm/include/llvm/MCA/CustomBehaviour.h:121
+  /// The description of Instrument kind
+  const llvm::StringRef Desc;
+
----------------
you're already in llvm namespace so you can drop "llvm::". Ditto for rest of the changes.


================
Comment at: llvm/include/llvm/MCA/CustomBehaviour.h:130
+
+  Instrument() : Desc(""), Data("") {}
+
----------------
`Instrument() = default` or even without one should suffice


================
Comment at: llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.cpp:49
+  // Return true if not one of the valid LMUL strings
+  return Data.equals("M1") || Data.equals("M2") || Data.equals("M4") ||
+         Data.equals("M8") || Data.equals("MF2") || Data.equals("MF4") ||
----------------
nit: maybe replace this with StringSwitch?


================
Comment at: llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.cpp:57
+  // These are the LMUL values that are used in RISCV tablegen
+  if (Data.equals("M1"))
+    return 0b000;
----------------
okay these lines should definitely be replaced with StringSwitch


================
Comment at: llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.cpp:78
+  // Currently, only support for RISCVLMULInstrument type
+  return Type.equals(RISCVLMULInstrument::DESC_NAME);
+}
----------------
nit: use `==` instead? (unlike Java `StringRef::operator==` and `StringRef::equals` are the same)


================
Comment at: llvm/tools/llvm-mca/CodeRegionGenerator.h:40
+
+  bool hadErr() { return FoundError; }
+};
----------------
nit: add const qualifier


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137440