[PATCH] D58728: [MCA] Highlight kernel bottlenecks in the summary view.

[Matt Davis via Phabricator via llvm-commits]

mattd accepted this revision.
mattd added a comment.
This revision is now accepted and ready to land.

I like this and think users will find it very helpful.  The changes seem sensible to me; however, I had a few nits... mostly stylistic things, no big deals.  Anyways, I'll mark this patch as  accept, as long as you cover the suggestions made by @lebedev.ri.



================
Comment at: include/llvm/MCA/Stages/ExecuteStage.h:33
+  unsigned NumIssuedOpcodes;
+  bool DispatchStallCycle;
+
----------------
Where is 'DispatchStallCycle' used?  I see it initialized, but not accessed anywhere in this patch.


================
Comment at: include/llvm/MCA/Stages/ExecuteStage.h:35
+
+  // True if this stage should notify pressure events to listeners.
+  bool EnablePressureEvents;
----------------
'True if this stage should notify listeners of HWPressureEvents'


================
Comment at: lib/MCA/HardwareUnits/Scheduler.cpp:186
         Strategy->compare(IR, ReadySet[QueueIndex])) {
-      const InstrDesc &D = IR.getInstruction()->getDesc();
+      Instruction &IS = *IR.getInstruction();
+      const InstrDesc &D = IS.getDesc();
----------------
As a side note, perhaps we should consider addding a getDesc() accessor to InstRef.  The pattern `getInstruction().getDesc()` or similar, is used throughout MCA.


================
Comment at: lib/MCA/HardwareUnits/Scheduler.cpp:250
+    }
+
+    RegDeps.emplace_back(IR);
----------------
Suggestion:  Remove the last 'continue' and add an 'else { RegDeps.emplace_back(IR); }'


================
Comment at: tools/llvm-mca/Views/SummaryView.cpp:28
     : SM(Model), Source(S), DispatchWidth(Width), LastInstructionIdx(0),
-      TotalCycles(0), NumMicroOps(0),
+      TotalCycles(0), NumMicroOps(0), BPI({0, 0, 0, 0}),
+      ResourcePressureDistribution(Model.getNumProcResourceKinds(), 0),
----------------
Perhaps we should introduce a constructor to `struct BackPressureInfo` that initializes the instance, instead of explicitly zeroing the members here.


================
Comment at: tools/llvm-mca/Views/SummaryView.cpp:70
+  switch (Event.Reason) {
+  case HWPressureEvent::INVALID:
+  default:
----------------
Seems like the case of INVALID should never occur.  If my assumption is true, should we toss in an assert here?


================
Comment at: tools/llvm-mca/llvm-mca.cpp:180
+    "bottleneck-analysis",
+    cl::desc("Enable bottleneck analysis (disabled by the fault)"),
+    cl::cat(ViewOptions), cl::init(false));
----------------
s/the fault/default/


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

https://reviews.llvm.org/D58728