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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Hi Michael,

Thanks a lot for this patch!

Two requests:

1. Please update the offical docs by adding a small paragraph (like we did for CustomBehaviour) describing this new feature and how to use it.
2. Please add some tests to verify that your new custom behaviour works as intended.

About 2.: you could reuse the code snippets from the summary of this review.
For each test, you can provide two (-instruction-tables) RUN lines:

- A RUN line with flag `-disable-im`.
- A RUN line without flag `-disable-im`.

That way, we can see how resource pressure changes when instruments are used to override the scheduling class IDs.

I also suggest to add a test where your special "instrument comments" are used in the presence of multiple nested code regions.
This last test is to ensure that lifetime of instruments is correctly handled, and that top-level instruments are correctly associated to nested regions too.

Thanks
-Andrea



================
Comment at: llvm/include/llvm/MCA/CustomBehaviour.h:119
+class Instrument {
+protected:
+  /// The description of Instrument kind
----------------
These fields should be private.




================
Comment at: llvm/include/llvm/MCA/CustomBehaviour.h:140-141
+
+/// This class creates Instruments and provides any behaviour,
+/// that MCA might need to perform by using said Instruments.
+class InstrumentManager {
----------------
This comment should be improved.

This class allows targets to optionally customize the logic that resolves scheduling class IDs.
Targets can use information encoded in so-called `Instrument` objects to make more informed scheduling decisions.

Something like that... Essentially, a brief description of what this class does.



================
Comment at: llvm/tools/llvm-mca/CodeRegion.h:29-54
+/// An instruction (a MCInst) is added to a CodeRegion R only if its
+/// location is in range [R.RangeStart, R.RangeEnd].
+///
+/// A InstrumentRegion describes a region of assembly code guarded by
+/// special LLVM-MCA comment directives.
+///
+///   # LLVM-MCA-<INSTRUMENTATION_TYPE> <data>
----------------
Could you please also update the llvm-mca documentation?


================
Comment at: llvm/tools/llvm-mca/llvm-mca.cpp:234-239
+static cl::opt<bool> DisableInstrumentManager(
+    "disable-im",
+    cl::desc(
+        "Disable instrumentation manager (use the default class which ignores instruments.)."),
+    cl::cat(ViewOptions), cl::init(false));
+
----------------
Please update the llvm-mca docs too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137440