[PATCH] D86644: [llvm-mca] Initial implementation of output serialization using JSON

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb requested changes to this revision.
andreadb added a comment.
This revision now requires changes to proceed.

The json output is better now.
However there are things about the implementation that must be fixed/changed (see my comments below).



================
Comment at: llvm/tools/llvm-mca/Views/View.cpp:30-39
+
+  json::Value InstructionViewJSONHandler::instToJSON() {
+    json::Array SourceInfo;
+    for (const auto &MCI : getSource()) {
+      StringRef Instruction = printInstructionString(MCI);
+      SourceInfo.push_back(Instruction.str());
+    }
----------------
Please move this to a separate .cpp file.


================
Comment at: llvm/tools/llvm-mca/Views/View.cpp:40-50
+  json::Value View::CPUtoJSON(StringRef MCPU,
+                              const llvm::MCSubtargetInfo &STI) {
+    json::Array Resources;
+    const MCSchedModel &SM = STI.getSchedModel();
+    for (unsigned I = 1, E = SM.getNumProcResourceKinds(); I < E; ++I) {
+      const MCProcResourceDesc &ProcResource = *SM.getProcResource(I);
+      Resources.push_back(ProcResource.Name);
----------------
This should not be a static member of View.
See my previous comment.


================
Comment at: llvm/tools/llvm-mca/Views/View.cpp:52-53
+
+  std::unique_ptr<InstructionViewJSONHandler>
+      InstructionViewJSONHandler::GlobalInstViewJSONHandler;
 } // namespace mca
----------------
There is already a mechanism for adding views. We shouldn't add things globally this way. See my other comments about `addView`.


================
Comment at: llvm/tools/llvm-mca/Views/View.h:35-37
+
+  static json::Value CPUtoJSON(StringRef MCPU,
+                               const llvm::MCSubtargetInfo &STI);
----------------
This should not be a static member of View. Please move this logic into a separate View class.

What we want is 'just yet-another-view' that prints out static information about the target CPU. That printing logic should be moved into a separate class; an instance of that class would then be added to the PipelinePrinter like any other view.



================
Comment at: llvm/tools/llvm-mca/Views/View.h:66-88
+
+// A special class to create the JSON value that describes the instructions.
+// There is only one such value and all the views refer to the individual
+// instructions by index.
+class InstructionViewJSONHandler : public InstructionView {
+  // Dummy definitions for pure virtual functions.
+  void printView(llvm::raw_ostream &) const {}
----------------
Please move this to a separate View header file.


================
Comment at: llvm/tools/llvm-mca/Views/View.h:81
+
+  ~InstructionViewJSONHandler() = default;
+
----------------
This should be a virtual destructor


================
Comment at: llvm/tools/llvm-mca/Views/View.h:87
+  // Pointer to a single global InstructionView object.
+  static std::unique_ptr<InstructionViewJSONHandler> GlobalInstViewJSONHandler;
+};
----------------
This is unnecessary.
See my comment in llvm-mca.cpp. We should just rely on `addView` to subscribe new views, rather than doing this.




================
Comment at: llvm/tools/llvm-mca/llvm-mca.cpp:569-572
+    if (PrintJson)
+      mca::InstructionViewJSONHandler::GlobalInstViewJSONHandler =
+          std::make_unique<mca::InstructionViewJSONHandler>(*STI, *IP, Insts);
+
----------------
You should be able to just call `addView` like we do for any other view in this file.

Something like:

```
Printer.addView(
          std::make_unique<mca::InstructionViewJSONHandler>(*STI, *IP, Insts));
```


================
Comment at: llvm/tools/llvm-mca/llvm-mca.cpp:576-580
+    if (PrintJson)
+      Printer.printJSON(TOF->os(), Insts, MCPU, *STI);
+    else
+      Printer.printReport(TOF->os());
 
----------------
I think that we should only keep a single API and avoid introducing method printJSON().

Regardless of whether the user requested a JSON report or not, the user should still be able to use the same API (i.e. just call `printReport(TOF->os())`)

You could add extra state to the PipelinePrinter (something like an `OutputKind` enum which, at the moment, is either default or json).

Also, we shouldn't be passing the MCInst array, MCPU, and STI back to printJSON.
The printer should be minimalistic and fully delegate to views the task of printing out stuff.



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

https://reviews.llvm.org/D86644