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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Hi,

Are you sure that you have uploaded the full diff?
A lot of things seem to be missing from the patch. For example, I cannot see where you add the string "Instructions and CPU resources". But that is just an example.

I left a few comments below.
However, you should re-upload this patch with full context (using -U999999 has documented here: https://releases.llvm.org/11.0.0/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

Thanks,
-Andrea



================
Comment at: llvm/docs/CommandGuide/llvm-mca.rst:214-216
+  The individual views refer to them by index. However, not all views are
+  supported. Unsupported views are not printed, such as bottleneck analysis,
+  etc.
----------------
"However, not all views are currently supported. For example, the report from the bottleneck analysis is not printed out in json. All the default views are currently supported."



================
Comment at: llvm/docs/CommandGuide/llvm-mca.rst:992
 Due to certain instructions not being expressed perfectly within their
-scheduling model, :program:`llvm-ma` isn't always able to simulate them
+scheduling model, :program:`llvm-mca` isn't always able to simulate them
 perfectly. Modifying the scheduling model isn't always a viable
----------------
thanks for catching this.


================
Comment at: llvm/test/tools/llvm-mca/JSON/X86/views.s:57-79
+# CHECK-NEXT:   "Instructions and CPU resources": {
+# CHECK-NEXT:     "Instructions": [
+# CHECK-NEXT:       "addl\t%eax, %eax",
+# CHECK-NEXT:       "addl\t%ebx, %ebx",
+# CHECK-NEXT:       "addl\t%ecx, %ecx",
+# CHECK-NEXT:       "addl\t%edx, %edx"
+# CHECK-NEXT:     ],
----------------
Why do we nest the name/value pairs for "Resources" and "Instructions" under "Instructions and CPU resources"?
Those should be at the top, and ideally they should not be nested under anything else.




================
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
----------------
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.


================
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]},
----------------
Please add a test for this.


================
Comment at: llvm/tools/llvm-mca/Views/TimelineView.h:128
   struct TimelineViewEntry {
-    int CycleDispatched;  // A negative value is an "invalid cycle".
+    int CycleDispatched; // A negative value is an "invalid cycle".
     unsigned CycleReady;
----------------
Please revert.


================
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"
 
----------------
Is this because of clang-format? Otherwise please revert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105064