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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

I don't like the test.
Can it simply be autogenerated, without omitting details



================
Comment at: llvm/test/tools/llvm-mca/JSON/X86/views.s:6
+
+# RUN: llvm-mca -mcpu=haswell --json -all-views=true < %s | FileCheck %s
+
----------------
Maybe decrease/hardcode iteration/cycle count.
I don't think you need that long of a simulation here.


================
Comment at: llvm/tools/llvm-mca/PipelinePrinter.cpp:30-31
+  for (const auto &V : Views) {
+    std::string VName = V->getNameAsString();
+    json::Value JV = V->toJSON();
+    JO.try_emplace(VName, std::move(JV));
----------------
I'd just inline them, less potential for dubious allocations.


================
Comment at: llvm/tools/llvm-mca/Views/BottleneckAnalysis.h:335
   void printView(raw_ostream &OS) const override;
+  std::string getNameAsString() const override { return "BottleneckAnalysis"; }
 
----------------
Are you sure these have to be `std::string`. I'd think they can be `StringLiteral`, or `StringRef`.


================
Comment at: llvm/tools/llvm-mca/Views/InstructionInfoView.cpp:130
+  if (IIVD.RThroughput.hasValue())
+    JO.try_emplace("RThroughput", IIVD.RThroughput.getValue());
+  else
----------------
`getValueOr("None")` ?


================
Comment at: llvm/tools/llvm-mca/llvm-mca.cpp:99
 
+static cl::opt<bool>
+    PrintJson("json",
----------------
I think there was some docs for the tool, it would be good to document this there, and possibly in releasenotes


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

https://reviews.llvm.org/D86644