[PATCH] D44309: [llvm-mca] Revactor event listeners to make the backend agnostic to event types.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Thanks Clement.

Please run clang-format on the patch. I found that the code was not correctly formatted in several places.
See my comments below.



================
Comment at: tools/llvm-mca/Backend.h:85
   unsigned getNumInstructions() const { return SM.size(); }
+  const Instruction& getInstruction(unsigned Index) const { return *Instructions.find(Index)->second; }
+  void eraseInstruction(unsigned Index) { Instructions.erase(Index); }
----------------
80-col.


================
Comment at: tools/llvm-mca/BackendStatistics.cpp:24-36
+  switch (Event.Type) {
+    case HWInstructionEvent::Retired:
+      ++NumRetired;
+      break;
+    case HWInstructionEvent::Issued:
+      ++NumIssued;
+      break;
----------------
clang-format.


================
Comment at: tools/llvm-mca/BackendStatistics.h:120-121
   void printView(llvm::raw_ostream &OS) const override {
-    printDispatchStalls(OS, B.getNumRATStalls(), B.getNumRCUStalls(),
-                        B.getNumSQStalls(), B.getNumLDQStalls(),
-                        B.getNumSTQStalls(), B.getNumDispatchGroupStalls());
+    printDispatchStalls(OS, B.getNumRATStalls(), B.getNumRCUStalls(), B.getNumSQStalls(),
+                        B.getNumLDQStalls(), B.getNumSTQStalls(), B.getNumDispatchGroupStalls());
     printRATStatistics(OS, B.getTotalRegisterMappingsCreated(),
----------------
Remove this change. Alternatively, run clang-format on this file.


================
Comment at: tools/llvm-mca/BackendStatistics.h:122-123
+                        B.getNumLDQStalls(), B.getNumSTQStalls(), B.getNumDispatchGroupStalls());
     printRATStatistics(OS, B.getTotalRegisterMappingsCreated(),
-                       B.getMaxUsedRegisterMappings());
+                           B.getMaxUsedRegisterMappings());
     printDispatchUnitStatistics(OS);
----------------
clang-format.


================
Comment at: tools/llvm-mca/Dispatch.cpp:154-160
+  DEBUG(dbgs() << "[E] Instruction Dispatched: " << Index << '\n');
+  Owner->notifyInstructionEvent(HWInstructionEvent(HWInstructionEvent::Dispatched, Index));
 }
 
 void DispatchUnit::notifyInstructionRetired(unsigned Index) {
-  Owner->notifyInstructionRetired(Index);
+  DEBUG(dbgs() << "[E] Instruction Retired: " << Index << '\n');
+  Owner->notifyInstructionEvent(HWInstructionEvent(HWInstructionEvent::Retired, Index));
----------------
80-col.


================
Comment at: tools/llvm-mca/HWEventListener.h:24
 
-class HWEventListener {
+// An event
+class HWInstructionEvent {
----------------
We need a better (doxygen) comment here.


================
Comment at: tools/llvm-mca/HWEventListener.h:27
 public:
-  // Events generated by the Retire Control Unit.
-  virtual void onInstructionRetired(unsigned Index) {};
+  // FIXME: This should be subtarget-specific. Move to TD files.
+  enum EventType {
----------------
Could you please explain what is the long term goal? Do you plan to remove this EventType?


================
Comment at: tools/llvm-mca/HWEventListener.h:42
+
+  // The event type. The exact meaning depends on the subtarget,
+  const unsigned Type;
----------------
Period at end of sentence.


================
Comment at: tools/llvm-mca/ResourcePressureView.cpp:48-50
+  if (Event.Type != HWInstructionEvent::Issued) {
+    return;
+  }
----------------
You don't need braces here.


================
Comment at: tools/llvm-mca/ResourcePressureView.h:62
 #include "View.h"
+#include "SourceMgr.h"
 #include "llvm/MC/MCInstPrinter.h"
----------------
Why this change?


================
Comment at: tools/llvm-mca/Scheduler.h:457-458
+
+  // The dispatch unit gets notified when instructions are executed.
+  DispatchUnit *DU;
 
----------------
Is the plan to remove this dependency in the long term?


Repository:
  rL LLVM

https://reviews.llvm.org/D44309