[PATCH] D105064: [llvm-mca] Fix JSON output

[Marcos Horro via Phabricator via llvm-commits]

markoshorro accepted this revision.
markoshorro marked 4 inline comments as done.
markoshorro added a comment.
This revision is now accepted and ready to land.

Thanks, @andreadb for the corrections. This is my first patch so I am sorry for these mistakes. I have left some comments inlined, however:

- String "Instructions and CPU resources" was already there, under Views/InstructionView.h

  StringRef getNameAsString() const override {
    return "Instructions and CPU resources";
  }



================
Comment at: llvm/docs/CommandGuide/llvm-mca.rst:215
+  The individual views refer to them by index, nonetheles, not all views are
+  printed when using in this format, e.g. bottleneck analysis, timeline view,
+  etc.
----------------
wolfgangp wrote:
> Don't we support the timeline view?
My bad, you are right.


================
Comment at: llvm/tools/llvm-mca/Views/BottleneckAnalysis.h:49-56
+/// +----> 1.    vhaddps %xmm2, %xmm2, %xmm3     ## RESOURCE interference:  JFPA
+/// [ probability: 73% ]
 /// +----> 2.    vhaddps %xmm3, %xmm3, %xmm4     ## REGISTER dependency:  %xmm3
 /// |
-/// |    < loop carried > 
+/// |    < loop carried >
 /// |
+/// +----> 1.    vhaddps %xmm2, %xmm2, %xmm3     ## RESOURCE interference:  JFPA
----------------
andreadb wrote:
> Please revert this change. It is incorrect. The probability information should always be on the same line as the instruction.
> 
> For the future: all these non-functional changes should be sent for review as a separate patch. They are not strictly related to what you are trying to fix. It just makes it harder to stay focused on the actual important changes.
You are right.


================
Comment at: llvm/tools/llvm-mca/Views/DispatchStatistics.cpp:87-96
+json::Value DispatchStatistics::toJSON() const {
+  json::Object JO({{"RAT", HWStalls[HWStallEvent::RegisterFileStall]},
+                   {"RCU", HWStalls[HWStallEvent::RetireControlUnitStall]},
+                   {"SCHEDQ", HWStalls[HWStallEvent::SchedulerQueueFull]},
+                   {"LQ", HWStalls[HWStallEvent::LoadQueueFull]},
+                   {"SQ", HWStalls[HWStallEvent::StoreQueueFull]},
+                   {"GROUP", HWStalls[HWStallEvent::DispatchGroupStall]},
----------------
andreadb wrote:
> Please add a test for this.
Sure, I forgot.


================
Comment at: llvm/tools/llvm-mca/Views/View.h:19-22
 #include "llvm/MCA/HWEventListener.h"
-#include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/JSON.h"
+#include "llvm/Support/raw_ostream.h"
 
----------------
andreadb wrote:
> Is this because of clang-format? Otherwise please revert.
Yes, this was because of clang-format, should I revert it anyways?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105064