[llvm-mca] Fix JSON output

[markoshorro]

Summary

Based on the discussion in [1], minor changes have been done to properly output a valid JSON. Removed "not implemented" keys.

[1] https://bugs.llvm.org/show_bug.cgi?id=50922

[wolfgangp]

llvm/docs/CommandGuide/llvm-mca.rst

214

May I suggest another wording:

The individual views refer to them by index. However, not all views are supported. Unsupported
views are not printed, such as bottleneck analysis, etc.

215

Don't we support the timeline view?

llvm/tools/llvm-mca/Views/TimelineView.h

184

Like in the comment above, I tought we implemented the timeline view.

[markoshorro]

Updated unit test and fixed TimelineView

[markoshorro]

Fixed test, added JSON check using the same approach followed in [1], as @gbedwell suggested in [2].

[1] https://github.com/llvm/llvm-project/blob/20df2c7052c09934ce87ccc409da9d3dc24b7ca0/llvm/test/Other/new-pm-time-trace.ll#L5
[2] https://bugs.llvm.org/show_bug.cgi?id=50922#c2

[andreadb]

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

llvm/docs/CommandGuide/llvm-mca.rst

214–216

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

992

thanks for catching this.

llvm/test/tools/llvm-mca/JSON/X86/views.s

57–79 ↗

(On Diff #355265)

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.

llvm/tools/llvm-mca/Views/BottleneckAnalysis.h

49–56

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.

llvm/tools/llvm-mca/Views/DispatchStatistics.cpp

87–96

Please add a test for this.

llvm/tools/llvm-mca/Views/TimelineView.h

128

Please revert.

llvm/tools/llvm-mca/Views/View.h

19–22

Is this because of clang-format? Otherwise please revert.

[markoshorro]

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";
}

llvm/docs/CommandGuide/llvm-mca.rst

215

My bad, you are right.

llvm/tools/llvm-mca/Views/BottleneckAnalysis.h

49–56

You are right.

llvm/tools/llvm-mca/Views/DispatchStatistics.cpp

87–96

Sure, I forgot.

llvm/tools/llvm-mca/Views/View.h

19–22

Yes, this was because of clang-format, should I revert it anyways?

[wolfgangp]

llvm/test/tools/llvm-mca/JSON/X86/views.s

57–79 ↗

(On Diff #355265)

I had put that in originally, so the instruction and resource list is treated like a view in its own right (not that it is one, just from a design point of view). Perhaps it needs to be redesigned?

llvm/tools/llvm-mca/Views/View.h

19–22

Yeah, generally we don't want to mix functional and (independent) formatting changes in the same patch. You're welcome to submit a second patch with just formatting changes, however.

[andreadb]

In D105064#2847881, @markoshorro wrote:

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

Sorry if I came across a bit too dry/direct.

In reality, I am very happy that you have sent this patch!

And yes, you are right that the string was already there. My mistake.

Speaking about your code comment fixes: most of them are actually good changes.
However - as Wolfgang also pointed out - it would be better if you could send them as a separate (follow-up) patch.

Thanks

llvm/test/tools/llvm-mca/JSON/X86/views.s

57–79 ↗

(On Diff #355265)

Mm.. I see. In that case, I think it is fine. It may not be worthy to change things too much.

[markoshorro]

Fixed test including DispatchStatistics. Removed format changes.

[markoshorro]

@andreadb I understand you were trying to go straight to the point, I am a newbie, and I need to learn :-)
When I have time, I will consider updating the other format changes in a new patch.

Thanks.

llvm/docs/CommandGuide/llvm-mca.rst

214–216

Noted.

llvm/test/tools/llvm-mca/JSON/X86/views.s

57–79 ↗

(On Diff #355265)

This is because they belong to InstructionView and since the printReport() function performs json::Object.try_emplace(View->getNameAsString().str(), View->toJSON()); for all Views, then it nests those pairs under that name. Should I change it anyways?

llvm/tools/llvm-mca/Views/View.h

19–22

Noted, good to know.

[andreadb]

LGTM.

Thanks Marcos!

[markoshorro]

@andreadb thanks for your guidance! I believe I do not have committ access, so I do not know if I have to ask for permission or if someone can do it on my behalf.
Again, thanks!

[andreadb]

In D105064#2851267, @markoshorro wrote:

@andreadb thanks for your guidance! I believe I do not have committ access, so I do not know if I have to ask for permission or if someone can do it on my behalf.
Again, thanks!

No problem,

I can commit this patch on your behalf.

Is it OK if I use your @udc.es email address, or do you prefer to use another email?

Thanks
-Andrea

[markoshorro]

Yes! [e-mail]
Thanks! And just to know, what is it needed to get commit access? I guess more contributions, isn't it?

[andreadb]

In D105064#2852475, @markoshorro wrote:

Yes! [e-mail]
Thanks! And just to know, what is it needed to get commit access? I guess more contributions, isn't it?

Have a look at this page: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

That link should hopefully answer to all your questions.

In my opinion, it makes sense to request for commit access if the plan is to keep contributing to the project.
My understanding is that it is mostly about how committed a developer is to the project, rather than how many commits have already been contributed.
But, that is just my personal interpretation of the process. As I wrote, you should be able to find "official" answers from that link :)

-Andrea

llvm/docs/CommandGuide/llvm-mca.rst

 .. option:: -bottleneck-analysis
 
   Print information about bottlenecks that affect the throughput. This analysis
-  can be expensive, and it is disabled by default.  Bottlenecks are highlighted
+  can be expensive, and it is disabled by default. Bottlenecks are highlighted
   in the summary view. Bottleneck analysis is currently not supported for
   processors with an in-order backend.
 
 .. option:: -json
 
-  Print the requested views in JSON format. The instructions and the processor
-  resources are printed as members of special top level JSON objects.  The
-  individual views refer to them by index.
-  
+  Print the requested views in valid JSON format. The instructions and the
+  processor resources are printed as members of special top level JSON objects.
+  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.
+
 .. option:: -disable-cb
 
   Force usage of the generic CustomBehaviour class rather than using the target

 Custom Behaviour
 """"""""""""""""""""""""""""""""""""
 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
 option though (maybe because the instruction is modeled incorrectly on
 purpose or the instruction's behaviour is quite complex). The

llvm/tools/llvm-mca/PipelinePrinter.cpp

 namespace mca {
 
 void PipelinePrinter::printReport(llvm::raw_ostream &OS) const {
-  for (const auto &V : Views)
-    V->printView(OutputKind, OS);
+  json::Object JO;
+  for (const auto &V : Views) {
+    if ((OutputKind == View::OK_JSON)) {
+      if (V->isSerializable()) {
+        JO.try_emplace(V->getNameAsString().str(), V->toJSON());
+      }
+    } else {
+      V->printView(OS);
+    }
+  }
+  if (OutputKind == View::OK_JSON)
+    OS << formatv("{0:2}", json::Value(std::move(JO))) << "\n";
 }
 } // namespace mca.
 } // namespace llvm

llvm/tools/llvm-mca/Views/BottleneckAnalysis.h

 /// Example of bottleneck analysis report for a dot-product on X86 btver2:
 ///
 /// Cycles with backend pressure increase [ 40.76% ]
-/// Throughput Bottlenecks: 
+/// Throughput Bottlenecks:
 ///   Resource Pressure       [ 39.34% ]
 ///   - JFPA  [ 39.34% ]
 ///   - JFPU0  [ 39.34% ]

 /// In particular, this occurs when there is a delta between the number of uOps
 /// dispatched and the number of uOps issued to the underlying pipelines.
 ///
-/// The bottleneck analysis view is also responsible for identifying and printing
-/// the most "critical" sequence of dependent instructions according to the
-/// simulated run.
+/// The bottleneck analysis view is also responsible for identifying and
+/// printing the most "critical" sequence of dependent instructions according to
+/// the simulated run.
 ///
 /// Below is the critical sequence computed for the dot-product example on
 /// btver2:

 
   void printView(raw_ostream &OS) const override;
   StringRef getNameAsString() const override { return "BottleneckAnalysis"; }
-  json::Value toJSON() const override { return "not implemented"; }
+  bool isSerializable() const override { return false; }
 
 #ifndef NDEBUG
   void dump(raw_ostream &OS, MCInstPrinter &MCIP) const { DG.dump(OS, MCIP); }

llvm/tools/llvm-mca/Views/DispatchStatistics.h

     printDispatchHistogram(OS);
   }
   StringRef getNameAsString() const override { return "DispatchStatistics"; }
+  json::Value toJSON() const override;
 };
 } // namespace mca
 } // namespace llvm

llvm/tools/llvm-mca/Views/DispatchStatistics.cpp

   OS << Buffer;
 }
 
+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]},
+                   {"USH", HWStalls[HWStallEvent::CustomBehaviourStall]}});
+  return JO;
+}
+
 } // namespace mca
 } // namespace llvm

llvm/tools/llvm-mca/Views/InstructionInfoView.cpp

     JO.try_emplace("Instruction", (unsigned)I.index());
     InstInfo.push_back(std::move(JO));
   }
-  return json::Value(std::move(InstInfo));
+  return json::Object({{"InstructionList", json::Value(std::move(InstInfo))}});
 }
 } // namespace mca.
 } // namespace llvm

llvm/tools/llvm-mca/Views/RegisterFileStatistics.h

   StringRef getNameAsString() const override {
     return "RegisterFileStatistics";
   }
+  bool isSerializable() const override { return false; }
 };
 } // namespace mca
 } // namespace llvm

llvm/tools/llvm-mca/Views/RetireControlUnitStatistics.h

   StringRef getNameAsString() const override {
     return "RetireControlUnitStatistics";
   }
+  bool isSerializable() const override { return false; }
 };
 
 } // namespace mca

llvm/tools/llvm-mca/Views/SchedulerStatistics.h

 
   void printView(llvm::raw_ostream &OS) const override;
   StringRef getNameAsString() const override { return "SchedulerStatistics"; }
+  bool isSerializable() const override { return false; }
 };
 } // namespace mca
 } // namespace llvm

llvm/tools/llvm-mca/Views/TimelineView.h

   unsigned LastCycle;
 
   struct TimelineViewEntry {
-    int CycleDispatched;  // A negative value is an "invalid cycle".
+    int CycleDispatched; // A negative value is an "invalid cycle".
     unsigned CycleReady;
     unsigned CycleIssued;
     unsigned CycleExecuted;

   }
   StringRef getNameAsString() const override { return "TimelineView"; }
   json::Value toJSON() const override;
+  bool isSerializable() const override { return false; }
 };
 } // namespace mca
 } // namespace llvm

llvm/tools/llvm-mca/Views/View.h

 
 #include "llvm/MC/MCInstPrinter.h"
 #include "llvm/MCA/HWEventListener.h"
-#include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/JSON.h"
+#include "llvm/Support/raw_ostream.h"
 
 namespace llvm {
 namespace mca {

   virtual ~View() = default;
   virtual StringRef getNameAsString() const = 0;
   virtual json::Value toJSON() const { return "not implemented"; }
+  virtual bool isSerializable() const { return true; }
   void anchor() override;
 };
 } // namespace mca