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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

I really like this JSON format. Thanks a lot!

The internal design however should be improved/simplified a bit (see my commet below).
This patch is almost ready to go; I plan to accept it once my comment below is addressed.

As a side note: shouldn't we add a line in the release notes about this new feature?
I don't actually remember the process for updating the release notes. When I firstly added llvm-mca, I remember I had to drop a line in a .rst file somewhere.
I think that this change is important enough to be advertised in the release notes (iirc there should be a section specifically for llvm tools). I don't know if the process has changed over the past few years though. Maybe @lebedev.ri and @RKSimon know more about it?

Thanks,
Andrea



================
Comment at: llvm/tools/llvm-mca/PipelinePrinter.cpp:16-24
+#include "llvm/Support/JSON.h"
 
 namespace llvm {
 namespace mca {
 
 void PipelinePrinter::printReport(llvm::raw_ostream &OS) const {
   for (const auto &V : Views)
----------------
In my previous comment I was suggesting to add the concept of "view output kind". A new enum type visible to all views (something like this):

```
enum class ViewOutputKind
{
  RAW,
  JSON
};
```

The PipelinePrinter could store that information as an internal field.
It could be done at construction time (or, if you really want, you could also add a new API like this `setOutputKind(enum ViewOutputKind VOK);`

The `printReport` API would still be unchanged. What would change though is the 
signature of `printView`, which would look like this:

```
void PipelinePrinter::printReport(llvm::raw_ostream &OS) const {
  for (const auto &V : Views)
    V->printView(OutputKind, OS);
}
```

The main advantage of this approach is that you don't need to derive new views just for printing a JSON report. It also avoids the visibility change "issue" (from `private` to `protected`) to expose the internal state of a base view class to the derived classes.

Method `printView` for most classes might look like this:

```
  void printView(llvm::View::FileOutputKind OK, llvm::raw_ostream &OS) const override
  {
    switch (OK)
   {
   default:
   case OK_RAW:
     printViewRaw(llvm::raw_ostream &OS);  /* a new private member of InstructionInfoView */
   case OK_JSON:
     printViewJSon(llvm::raw_ostream &OS);  /* a new private member of InstructionInfoView */
   }
  }
```

`printViewRaw` and `printViewJSon` would be private members of a view.

That way, you don't need separate classes just for the purpose of printing the report in a different format. The logic that prints json files would be fully contained in the `printViewJSon`.

This might also simplify a bit how the PipelinePrinter is populated (though I am not 100% sure about this).


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

https://reviews.llvm.org/D86644